Feat/add endpoints for uploading adverse actions for licenses and privileges #769
Conversation
WalkthroughThis change introduces a comprehensive adverse action (encumbrance) feature for provider licenses and privileges within the Compact Connect system. It adds new API endpoints for state administrators to upload adverse actions, implements the backend logic to persist these encumbrances, and updates the data models and schemas to support adverse action records. The update includes new data classes, Marshmallow schemas, Lambda handler logic, CloudFormation resource definitions, and extensive unit and integration tests. Test utilities and constants are added to facilitate consistent test data generation, and existing provider and privilege/license models are refactored to support adverse actions and their associations. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUser
participant API Gateway
participant EncumbranceLambda
participant DataClient
participant DynamoDB
AdminUser->>API Gateway: POST /providers/{id}/privileges/.../encumbrance (JSON body)
API Gateway->>EncumbranceLambda: Invoke with path/body/context
EncumbranceLambda->>DataClient: encumber_privilege(adverse_action)
DataClient->>DynamoDB: Transactional write (adverse action, privilege update, privilege status)
DynamoDB-->>DataClient: Success/Failure
DataClient-->>EncumbranceLambda: Result
EncumbranceLambda-->>API Gateway: 200 OK (Success message)
API Gateway-->>AdminUser: Response
Note over EncumbranceLambda,DataClient: Similar flow for license encumbrance via encumber_license
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (28)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
38-56: Constructor does a costly dump→load round-trip and partially validates DB keys
__init__first serializes the supplieddatawithrecord_schema.dump()only to immediately de-serialize it again withload().
This double pass:
- Performs duplicate (de)serialization work on every new object creation,
- Silently discards unknown fields during the
dump()step (Marshmallow’s default is to drop them), possibly masking bad input before schema validation,- Adds no extra validation beyond what a single
load()already provides.You can achieve the same effect—and catch unknown fields—by doing one
load()call, then invoking a schema method (e.g.generate_gsis()) if you truly need post-processing.Example diff:
- serialized_object = self._record_schema.dump(data) - self._data = self._record_schema.load(serialized_object) + # A single load both validates and normalises. If additional post- + # processing (e.g. GSI generation) is required, call it explicitly. + self._data = self._record_schema.load(data) + self._data = self._record_schema.post_dump(self._data, many=False)
58-70: Expose safer fallback for missing mandatory fields
typeanddateOfUpdateaccessself._data[...]directly, raisingKeyErrorif the underlying dict is incomplete (e.g. object instantiated empty then not populated).
Consider returningNoneor a custom exception with a clearer message.
195-205: Long descriptive strings inClinicalPrivilegeActionCategoryStoring full NPDB descriptions as enum values increases storage size in DynamoDB keys/indices. If these appear in primary or GSI keys, consider keeping concise machine-friendly values (e.g.
FRAUD) and mapping to human-readable labels at the API layer.backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json (1)
1-24: Define privilege encumbrance request schema
The new schema correctly specifies the three required fields and disallows additional properties, aligning with the encumbrance handler’s expectations.Optionally, add a
"format": "date"or a regex pattern toencumbranceEffectiveDateto enforce ISO-8601 date compliance:"encumbranceEffectiveDate": { "description": "The effective date of the encumbrance", "type": "string", + "format": "date" }backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json (1)
1-24: Schema definition is well-structured for license encumbrance requests.The schema properly defines the required fields for license encumbrance (adverse action) requests with appropriate descriptions and type constraints. This aligns with the PR objective of adding endpoints for uploading adverse actions against licenses.
However, consider adding pattern validation for the
encumbranceEffectiveDatestring to ensure it matches the expected date format."encumbranceEffectiveDate": { "description": "The effective date of the encumbrance", - "type": "string" + "type": "string", + "pattern": "^\\d{4}-\\d{2}-\\d{2}$" },backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py (1)
1-32: Consider using direct dictionary access instead of.get()for property getters.The implementation of
HomeJurisdictionSelectionDatafollows the pattern of other data classes, but the property getters useself._data.get('key')which will returnNoneif the key doesn't exist, potentially hiding errors. If these properties are expected to always exist, consider using direct dictionary accessself._data['key']to raise aKeyErrorif the key is missing, making debugging easier.Also, consider expanding the class docstring to be more specific about what a "Home Jurisdiction Selection" represents in the context of the application.
class HomeJurisdictionSelectionData(CCDataClass): """ Class representing a Home Jurisdiction Selection with properties and setters. + + A Home Jurisdiction Selection represents the jurisdiction a provider has + designated as their primary location for compact purposes. """ def __init__(self, data: dict[str, Any] = None): super().__init__(ProviderHomeJurisdictionSelectionRecordSchema(), data) @property def provider_id(self) -> UUID: - return self._data.get('providerId') + return self._data['providerId'] @property def compact(self) -> str: - return self._data.get('compact') + return self._data['compact'] @property def jurisdiction(self) -> str: - return self._data.get('jurisdiction') + return self._data['jurisdiction'] @property def date_of_selection(self) -> datetime: - return self._data.get('dateOfSelection') + return self._data['dateOfSelection']backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
113-114: Ensure type validation in administrator_set_status setterThe setter method accepts a parameter of type
ActiveInactiveStatusbut doesn't validate that the input is of this type, potentially allowing strings or other values to be set directly.Consider adding type validation to ensure the parameter is actually an
ActiveInactiveStatusenum value:@administrator_set_status.setter def administrator_set_status(self, value: ActiveInactiveStatus) -> None: + if not isinstance(value, ActiveInactiveStatus): + value = ActiveInactiveStatus(value) self._data['administratorSetStatus'] = valuebackend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
411-440: Extract common schema to reduce duplicationThe
post_license_encumbrance_request_modelandpost_privilege_encumbrance_request_modelhave identical structure and fields, indicating an opportunity for code reuse.Create a shared schema definition:
@property def _encumbrance_request_schema(self): return JsonSchema( type=JsonSchemaType.OBJECT, additional_properties=False, required=['encumbranceEffectiveDate', 'clinicalPrivilegeActionCategory', 'blocksFuturePrivileges'], properties={ 'encumbranceEffectiveDate': JsonSchema( type=JsonSchemaType.STRING, description='The effective date of the encumbrance', format='date', pattern=cc_api.YMD_FORMAT, ), 'clinicalPrivilegeActionCategory': JsonSchema( type=JsonSchemaType.STRING, description='The category of clinical privilege action', enum=['SUSPENSION', 'REVOCATION', 'LIMITATION', 'OTHER'], ), 'blocksFuturePrivileges': JsonSchema( type=JsonSchemaType.BOOLEAN, description='Whether this encumbrance blocks future privileges', ), }, )Then use it in both properties:
@property def post_privilege_encumbrance_request_model(self) -> Model: """Return the post privilege encumbrance request model, which should only be created once per API""" if hasattr(self.api, '_v1_post_privilege_encumbrance_request_model'): return self.api._v1_post_privilege_encumbrance_request_model self.api._v1_post_privilege_encumbrance_request_model = self.api.add_model( 'V1PostPrivilegeEncumbranceRequestModel', description='Post privilege encumbrance request model', schema=self._encumbrance_request_schema, ) return self.api._v1_post_privilege_encumbrance_request_modelbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (1)
53-55: Inconsistency with class documentationWhile the class documentation (lines 13-15) states that the class does not include setters because license records are only upserted during ingestion, a setter is provided for
given_name.Either:
- Update the documentation to clarify that some setters exist for specific fields:
""" Class representing a License with getters and setters for all properties. Unlike several other CCDataClass subclasses, this one does not include setters for most properties. This is because license records are only upserted during ingestion, so we can pass the entire record from the ingestion process into the constructor. """
- Or remove the setter for consistency:
-@given_name.setter -def given_name(self, value: str) -> None: - self._data['givenName'] = valuebackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
193-198: Add defensive error handling for adverse action recordsThe adverse action handling assumes that records have both "jurisdiction" and "licenseType" fields, but doesn't validate their presence, which could lead to KeyError exceptions.
Add defensive error handling:
case 'adverseAction': logger.debug('Identified adverse action record') + try: if record['actionAgainst'] == 'privilege': privileges[f'{record["jurisdiction"]}-{record["licenseType"]}']['adverseActions'].append(record) elif record['actionAgainst'] == 'license': licenses[f'{record["jurisdiction"]}-{record["licenseType"]}']['adverseActions'].append(record) + except KeyError as e: + logger.warning(f"Adverse action record missing required fields: {e}") + # Decide whether to raise an exception or skip the recordbackend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
75-85: Simplify & short-circuit licence-type look-upThe current loop keeps iterating even after finding a match. A
dictcomprehension ornext()is clearer and O(n)→early-exit.- adverse_action_license_type = None - adverse_action_license_type_abbreviation = None - - for license_type_name, license_type_abbreviation in compact_license_types.items(): - if license_type_abbreviation_from_path_parameter == license_type_abbreviation.lower(): - adverse_action_license_type = license_type_name - adverse_action_license_type_abbreviation = license_type_abbreviation + match = next( + ( + (name, abbr) + for name, abbr in compact_license_types.items() + if abbr.lower() == license_type_abbreviation_from_path_parameter + ), + None, + ) + if match: + adverse_action_license_type, adverse_action_license_type_abbreviation = matchbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
14-19: Export constants in an__all__to guide static analyzersAdding
__all__makes public API intent explicit and prevents * import leaking.MILITARY_AFFILIATION_RECORD_TYPE = 'militaryAffiliation' +# Public symbols re-exported by this module +__all__ = [ + 'MilitaryAffiliationStatus', + 'MilitaryAffiliationType', + 'SUPPORTED_MILITARY_AFFILIATION_FILE_EXTENSIONS', + 'MILITARY_AFFILIATIONS_DOCUMENT_TYPE_KEY_NAME', + 'MILITARY_AFFILIATION_RECORD_TYPE', +]backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
346-358: Typo in test name –returs→returnsAccurate test names aid discoverability.
- def test_patch_provider_military_affiliation_returs_400_if_invalid_body(self): + def test_patch_provider_military_affiliation_returns_400_if_invalid_body(self):backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (2)
59-65: Getter / setter symmetry foraction_againstThe setter expects an
AdverseActionAgainstEnumbut the getter returns a rawstr.
Call-sites therefore have to remember to convert back to the Enum if they want to compare values, which is error-prone and undermines the type-safety you were aiming for.Consider returning the Enum instead:
- def action_against(self) -> str: - return self._data.get('actionAgainst') + def action_against(self) -> AdverseActionAgainstEnum | None: + value = self._data.get('actionAgainst') + return AdverseActionAgainstEnum(value) if value is not None else NoneThat way the API remains symmetrical and consumers consistently work with the Enum.
121-126: Setter type hint should allowNone
lifting_useris nullable (getter annotation already reflects this) but the setter’s parameter type isUUID, preventing callers from clearing the field without a cast.- def lifting_user(self, value: UUID) -> None: + def lifting_user(self, value: UUID | None) -> None:This keeps the public contract consistent.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py (2)
85-88:adverseActionslist may be omitted – consider default empty listThe field is marked
required=False/allow_none=False.
If the key is omitted entirely, Marshmallow accepts the object; if the key is present butnull, validation fails.
Many clients find it more convenient when collections are ALWAYS present (as[]) to avoid extra null-checks.You can achieve that by adding
missing=list:- adverseActions = List(Nested(AdverseActionGeneralResponseSchema, required=False, allow_none=False)) + adverseActions = List( + Nested(AdverseActionGeneralResponseSchema, allow_none=False), + required=False, + allow_none=False, + missing=list, + )Same comment applies to the public schema below.
153-155: Duplication between public & staff schemasBoth
PrivilegeGeneralResponseSchemaandPrivilegePublicResponseSchemanow declare an identicaladverseActionsfield.
To avoid drift, you could pull this into a small mix-in or base class shared by both privilege schemas (similar to howhistoryis handled).backend/compact-connect/tests/app/test_api/test_provider_management_api.py (2)
475-490: Missing negative-path assertion for Lambda handler nameYou assert the handler path for the privilege encumbrance Lambda but do not check the same for the license encumbrance flow added later in the test file.
A simple copy-paste of the block below (adapted to the license resource) would guard against future regressions where the handler logical-ID changes silently.
491-563: Highly repetitive resource-path discovery – extract helperThe four nested
find_resourcescalls (privileges→jurisdiction→ …) are repeated almost verbatim in several tests (deactivate privilege, privilege encumbrance, license encumbrance).
Boiler-plate makes the test brittle and obscures intent.Consider introducing a helper that walks the template given a list of
PathParts and returns the logical-ID of the last resource, e.g.:def get_resource_id(template: Template, parent_id: str, parts: list[str]) -> str: current_parent = parent_id for part in parts: resources = template.find_resources( CfnResource.CFN_RESOURCE_TYPE_NAME, props={'Properties': {'ParentId': {'Ref': current_parent}, 'PathPart': part}}, ) assert len(resources) == 1 current_parent = next(iter(resources)) return current_parentYour tests then shrink to:
encumbrance_resource_id = get_resource_id( api_stack_template, api_stack.get_logical_id(provider_resource), ['privileges', 'jurisdiction', '{jurisdiction}', 'licenseType', '{licenseType}', 'encumbrance'], )This keeps each test focused on the behaviour being validated, not on the plumbing required to navigate the template.
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py (1)
78-90: Test could silently pass with a non-UUID value
assertIsInstance(dumped_data['adverseActionId'], str)only guarantees a string, not a UUID.
Usinguuid.UUID()for parsing enforces the type and format:- self.assertIsInstance(dumped_data['adverseActionId'], str) + from uuid import UUID + # Will raise ValueError if not a valid UUID string + UUID(dumped_data['adverseActionId'])backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (2)
573-587: Docstring inaccurate – refers to deactivation instead of encumbranceThe helper builds the Lambda powering the encumbrance endpoints, but the docstring still says “deactivating a provider's privilege”, which is misleading for future maintainers.
- """Create and configure the Lambda handler for deactivating a provider's privilege.""" + """Create and configure the Lambda handler for encumbering a provider's license or privilege."""
608-656: Consider dedicated handlers or explicit request contextBoth privilege- and license-level encumbrance routes invoke the same Lambda (
ProviderEncumbranceHandler).
Ensure the handler reliably distinguishes the target (privilege vs. license) usingpathParameters; otherwise an incorrect branch could be updated.If the logic diverges substantially, two thin handlers delegating to shared code may simplify IAM scoping and CloudWatch metrics.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
44-52: Enum instance vs. value – explicit string conversion improves readability
ClinicalPrivilegeActionCategory.UNSAFE_PRACTICEis astr-Enum, sojson.dumpsworks, but the implicit coercion is opaque.
Passing.value(or casting tostr) makes intent crystal-clear and avoids surprises if the enum base type changes.- 'clinicalPrivilegeActionCategory': ClinicalPrivilegeActionCategory.UNSAFE_PRACTICE, + 'clinicalPrivilegeActionCategory': ClinicalPrivilegeActionCategory.UNSAFE_PRACTICE.value,
59-76: Hidden duplication – consider parameterising helper
_when_testing_valid_privilege_encumbrance&_when_testing_valid_license_encumbranceonly differ in the resource string &
event-specific record generator. Extract a generic helper (or usepytest.mark.parametrize) to cut duplication and future drift.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
924-977: Duplicate helpers – consolidate_generate_put_adverse_action_item&_generate_put_item_transactionBoth helpers wrap a
Putoperation with identical structure; the only difference is that one expects an
AdverseActionDatainstance while the other accepts a plain dict. Keeping both increases maintenance cost
and risks divergence.Idea: drop
_generate_put_adverse_action_itemand always call the generic_generate_put_item_transaction,
doing theserialize_to_database_record()call at the call-site when needed.
987-1078: Encumber privilege – conditional update is non-idempotentIf two concurrent encumbrance requests arrive for the same privilege, both will attempt
to write the sameprivilegeUpdate&adverseActionrecords (identical PK/SK collisions) and, when the privilege is already inactive, the second transaction still inserts a second duplicate update record with an emptyupdatedValuesdict.Consider:
- Adding an
ConditionExpressionto thePuton the update & adverse-action items (attribute_not_exists(pk)) to guarantee
at-most-once semantics.- Using a deterministic UUID or hash for
adverseActionIdto make retries idempotent.
1079-1169: Docstring & log messages mention “privilege”, but method operates on licensesMinor, yet confusing:
encumber_licensecopies the docstring fromencumber_privilege, repeatedly
saying “privilege”. Update the wording to “license” to avoid cognitive overhead for future maintainers.backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
228-232: Consider consistent naming for abbreviation methods.The method name uses "abbr" while working with "abbreviations" in the config. Consider using the full word for consistency.
-def get_license_type_abbr_for_license_type(compact: str, license_type: str) -> str: +def get_license_type_abbreviation_for_license_type(compact: str, license_type: str) -> str: from cc_common.config import config return config.license_type_abbreviations[compact][license_type]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
backend/compact-connect/lambdas/python/common/cc_common/config.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(6 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/__init__.py(1 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(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py(5 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py(5 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/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json(1 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json(2 hunks)backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json(1 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/provider-data-v1/handlers/encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py(7 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(5 hunks)backend/compact-connect/tests/app/test_api/test_provider_management_api.py(17 hunks)backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_DEACTIVATION_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (15)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py (1)
LicenseCommonSchema(15-44)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (2)
MilitaryAffiliationStatus(4-7)MilitaryAffiliationType(10-12)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationType(10-12)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (4)
ActiveInactiveStatus(180-182)ClinicalPrivilegeActionCategory(195-205)CompactEligibilityStatus(185-187)UpdateCategory(171-177)
backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
ProviderManagement(34-656)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
compact(26-27)compact(148-149)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (4)
compact(29-30)compact(33-34)compact(149-150)compact(153-154)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
AdverseActionGeneralResponseSchema(51-59)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
ForgivingSchema(23-27)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (4)
NationalProviderIdentifier(30-32)ITUTE164PhoneNumber(60-66)ActiveInactive(45-47)CompactEligibility(50-52)
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (4)
backend/compact-connect/tests/app/base.py (2)
get_resource_properties_by_logical_id(65-72)compare_snapshot(393-420)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(28-36)backend/compact-connect/stacks/api_stack/cc_api.py (1)
staff_users_authorizer(247-253)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
message_response_model(21-39)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
LicenseUpdatePreviousResponseSchema(177-206)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py (1)
PrivilegeUpdatePreviousGeneralResponseSchema(28-45)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (2)
AdverseActionGeneralResponseSchema(51-59)AdverseActionPublicResponseSchema(24-48)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (2)
ActiveInactive(45-47)Jurisdiction(40-42)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCEnum(95-105)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (4)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
TstFunction(22-394)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
ClinicalPrivilegeActionCategory(195-205)serialize_to_database_record(89-92)to_dict(76-87)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (8)
put_default_privilege_record_in_provider_table(220-226)get_license_type_abbr_for_license_type(229-232)generate_default_adverse_action(48-68)generate_default_privilege_update(235-260)generate_default_privilege(189-211)put_default_license_record_in_provider_table(154-160)generate_default_license_update(163-186)generate_default_license(117-151)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(28-36)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (1)
PostMilitaryAffiliationResponseSchema(11-23)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (2)
ActiveInactive(45-47)Jurisdiction(40-42)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (1)
Jurisdiction(44-90)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
attestations(93-94)attestations(97-98)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCDataClass(15-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/record.py (1)
ProviderHomeJurisdictionSelectionRecordSchema(10-25)
🔇 Additional comments (52)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
162-169: Confirm downstream handling for newAdverseActionAgainstEnumAdding a brand-new enum is fine, but any
.from_str()or literal comparisons in existing code paths will now receive unexpected values (privilege/license). Please verify usage sites to avoidKeyError/ValueError.
171-178: Enum re-ordering changes string values order only — good, but double-check DB migrationsBecause
UpdateCategoryis aStrEnum, only the names / values matter (not the declaration order). Still, if you persist the.nameof an enum anywhere, inserting new members may break deserialization of historical records. Make sure historical data is covered by tests/migrations.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py (1)
119-119: Import path update looks correctThe test now imports
MilitaryAffiliationTypefrom the new...military_affiliation.commonmodule, matching the refactor. No further action required.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (1)
8-8: Updated import path aligns with new module structureSwitching to
military_affiliation.commonkeeps the API schema in sync with the enum relocation. Looks good.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py (1)
15-15: Import path update is correct and aligns with refactor
TheLicenseCommonSchemaimport now points to the consolidatedlicense/common.pymodule, matching the new schema layout. No changes needed beyond this alignment.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py (1)
24-24: Consistent import path adjustment for LicenseCommonSchema
The import ofLicenseCommonSchemafrom thelicense/common.pysubmodule mirrors the change in the ingest schema and maintains modularity. This aligns with the broader refactoring.backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.py (1)
3-3: Align military affiliation constant import with new module structure
TheMILITARY_AFFILIATIONS_DOCUMENT_TYPE_KEY_NAMEconstant is now imported from themilitary_affiliation/common.pysubmodule, consistent with the refactoring in related files.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py (1)
8-12: Update military affiliation enums import to common submodule
TheMILITARY_AFFILIATION_RECORD_TYPE,MilitaryAffiliationStatus, andMilitaryAffiliationTypeare now sourced frommilitary_affiliation/common.py, matching the new centralized definitions. This import change is correct.backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json (2)
61-61: IncludeadverseActionsin license objects
Ensures that the license schema in test fixtures aligns with the newLicenseGeneralResponseSchemawhich now exposes an emptyadverseActionslist when there are no adverse actions.Please verify in
provider_record_util.pyand the corresponding Marshmallow schema thatadverseActionsis always present (defaulting to[]) for license entries even when no actions exist.
118-118: IncludeadverseActionsin privilege objects
Adds an emptyadverseActionsarray to privilege fixtures to match the updatedPrivilegeGeneralResponseSchema.Confirm that the privilege encumbrance logic populates this field consistently and that the API response schema reflects it.
backend/compact-connect/tests/resources/snapshots/PRIVILEGE_DEACTIVATION_REQUEST_SCHEMA.json (1)
10-12: EnforcedeactivationNoteas a required property
Adding"required": ["deactivationNote"]ensures that any privilege deactivation request must include a descriptive note.Verify that the handler in
privileges.pycorrectly rejects requests withoutdeactivationNoteand returns a clear validation error.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (1)
116-116: Clarify omission of licenses in public schema
The added comment makes it explicit thatProviderPublicResponseSchemaintentionally omits license data, helping future maintainers understand the exposure differences between public and staff endpoints.backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json (1)
1-5: Add sample adverse action POST payload
Provides a concrete example for testing the new encumbrance endpoint with the required fields:encumbranceEffectiveDate,clinicalPrivilegeActionCategory, andblocksFuturePrivileges.Consider adding additional fixtures for edge cases (e.g., invalid dates or missing required fields) to strengthen schema validation tests.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (1)
6-11: Import organization looks good.The imports have been reorganized to align with the broader refactoring of the military affiliation data model, where schemas are imported from the
.apisubmodule and constants/enums from the.commonsubmodule.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/__init__.py (1)
3-12: Schema imports look good and align with the new adverse action functionality.The new imports properly register all the necessary schema record types that support the adverse action functionality and other data model changes introduced in this PR.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
4-7: Import cleanup looks good.The removal of the
pre_loadimport aligns with the refactoring of schema processing mentioned in the PR objectives. This simplifies the schema by removing redundant datetime conversion.
66-103:✅ Verification successful
Verify impact of removing pre_load_initialization method.
The file appears to have removed a
@pre_loadmethod that previously converted theproviderDateOfUpdatefield to a datetime object. Ensure that this conversion is now handled elsewhere in the data model abstraction, especially since line 64 still definesproviderDateOfUpdateas a DateTime field.
🏁 Script executed:
#!/bin/bash # Search for any other handling of providerDateOfUpdate conversion rg "providerDateOfUpdate" --type py -A 5 -B 5Length of output: 25998
No missing conversion for
providerDateOfUpdateThe Marshmallow
DateTimefield onproviderDateOfUpdateautomatically deserializes ISO-formatted strings intodatetimeobjects on load and serializes them back to strings on dump, so the former@pre_loadhook is no longer needed. The current@pre_dumpand@post_loadhandlers correctly set and then remove the dynamic field.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
69-71: LGTM! Your implementation matches the pattern used by other custom fields.The new
ClinicalPrivilegeActionCategoryFieldclass correctly validates string values against theClinicalPrivilegeActionCategoryenum and follows the same implementation pattern as other similar field classes likeActiveInactiveandUpdateType.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (1)
142-145: LGTM! Field reordering doesn't impact functionality.The reordering of fields in
PrivilegeUpdatePreviousRecordSchemamaintains consistent field types and validation logic while improving alignment with related schemas in the codebase.Also applies to: 150-150
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py (2)
28-30: LGTM! Good testing improvement.Setting
self.maxDiff = Noneensures that test failures will show complete diffs regardless of object size, which is very helpful for debugging complex test failures.
33-37: LGTM! Good organization of environment-dependent imports.Moving environment-dependent imports inside the setup method ensures they're only imported after the
TstLambdasinitialization has configured the required environment variables. The comment clearly explains the rationale, and theTestDataGeneratoris properly assigned for use in tests.Also applies to: 40-40
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
149-150: Good addition of helper method for accessing license type abbreviations by compact.This method provides a clean interface to access the license type abbreviations dictionary for a specific compact, consistent with the existing
license_types_for_compactmethod.
210-213: Good refactoring of timezone handling.Extracting the timezone into a separate property improves maintainability by centralizing the UTC-4 timezone definition. This makes it easier to modify the timezone in the future if needed and follows the DRY principle by avoiding duplication.
Also applies to: 222-222
backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
9-9: Clean refactoring from QueryProviders to ProviderManagement.This change appropriately replaces the
QueryProvidersimplementation with the more comprehensiveProviderManagementclass while maintaining the same interface and constructor arguments. This is well-aligned with the PR's goal of adding adverse action functionality to providers, licenses, and privileges.Also applies to: 139-140
backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py (1)
45-45: Consistent with updated requirement for deactivationNote.The usage of
deactivation_notefrom the required field is correctly propagated here.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py (1)
1-45: Well-structured schema with clear field definitions and validation rules.The
LicenseCommonSchemaprovides a solid foundation for license data validation with:
- Clear separation between required and optional fields
- Appropriate validators for field lengths
- Type-specific fields where needed (e.g., Date, Email, ITUTE164PhoneNumber)
- Good documentation explaining the purpose and serialization direction
This schema will help ensure consistent data quality across the application.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
9-125: Well-structured data class implementation for provider records.The
ProviderDataclass follows good practices for data encapsulation:
- Inherits from
CCDataClassfor consistent behavior with other data models- Provides typed, read-only property accessors for all fields
- Handles optional fields appropriately with
.get()and default values- Includes descriptive docstrings explaining the purpose and data source
This implementation will improve code maintainability by providing a structured interface to provider data rather than direct dictionary access.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (3)
11-11: LGTM: Import added for adverse action integration.The import of
AdverseActionGeneralResponseSchemais needed for the new adverse actions feature.
174-174: LGTM: Added adverse actions to license response schema.This change appropriately extends the license schema to include associated adverse actions, supporting the new encumbrance feature.
177-206: LGTM: New schema for detailed license history snapshots.The
LicenseUpdatePreviousResponseSchemaproperly implements a detailed snapshot for license history, including:
- All fields have appropriate validation rules
- Sensitive fields like SSN and DOB are included for users with 'readPrivate' permission
- Clear docstrings explaining purpose and serialization direction
This aligns with the PR's data model enhancements for tracking license changes.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (1)
11-46: LGTM: Well-implemented data class for military affiliations.The
MilitaryAffiliationDataclass follows the same excellent pattern as other data classes:
- Inherits from
CCDataClassfor consistent behavior- Provides typed property accessors for all fields
- Uses
.get()with appropriate defaults for optional fields and collections- Clean and consistent implementation with good type annotations
This refactoring improves code structure by moving from direct dictionary access to a more object-oriented approach.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (3)
9-22: LGTM: Well-designed POST request schema for adverse actions.The
AdverseActionPostRequestSchemaproperly defines the required fields for creating adverse actions:
- Required date, category, and boolean flag fields with appropriate validations
- Clear docstrings explaining purpose and serialization direction
- Extends ForgivingSchema to handle unknown fields appropriately
This schema validates the inputs needed for the new encumbrance endpoints.
24-49: LGTM: Comprehensive public response schema for adverse actions.The
AdverseActionPublicResponseSchemaincludes all necessary fields for public API responses:
- Core identifier fields (type, compact, providerId, jurisdiction)
- License type information
- Flag for blocking future privileges
- Creation and update timestamps
- Optional lift date for when actions are removed
The schema properly distinguishes between required creation fields and optional fields that are populated when an action is lifted.
51-59: LGTM: Extended schema for users with additional permissions.The
AdverseActionGeneralResponseSchemaappropriately extends the public schema by adding theclinicalPrivilegeActionCategoryfield, which appears to require elevated permissions. This properly implements the permission-based schema hierarchy.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
12-119: Privilege class implementation follows good OOP principlesThe
PrivilegeDataclass shows good encapsulation by implementing typed getters and setters for all properties, properly wrapping the schema validation provided by the MarshmallowPrivilegeRecordSchema.
124-197: PrivilegeUpdateData implementation is consistent and well-structuredThe
PrivilegeUpdateDataclass appropriately models privilege update operations with clearly defined properties, proper typing, and good documentation.backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
365-367: Good - Requiring deactivation note ensures proper documentationMaking the
deactivationNoterequired ensures administrators provide reasons for deactivating privileges, improving audit trails and accountability.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
9-16: Clear documentation about the lack of settersThe class documentation clearly explains why setters are generally not provided, which is helpful for understanding the design decision.
130-169: LicenseUpdateData class is well-structuredThe
LicenseUpdateDataclass appropriately models license update operations with clearly defined properties, proper typing, and good documentation.backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
32-33: Good initialization of schema instances at class levelCreating the schema instances at class level avoids repetitive instantiation and is a good optimization for frequently used schemas.
171-176: Proper initialization of adverseActions listsInitializing empty lists for
adverseActionsalongside the existinghistorylists ensures consistent data structure.
231-239: Direct enum comparison is cleaner than string comparisonComparing directly against enum members rather than their string values is safer and more maintainable.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
90-118: UseassertCountEqualfor collection comparison to avoid order sensitivity
provider_data['licenses'][0]['adverseActions']ordering is not guaranteed by DynamoDB reads. A deterministic failure may occur.- self.assertEqual( - [self.test_data_generator.convert_data_to_api_response_formatted_dict(test_adverse_action)], - license_adverse_actions, - ) + self.assertCountEqual( + [self.test_data_generator.convert_data_to_api_response_formatted_dict(test_adverse_action)], + license_adverse_actions, + )backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (1)
44-53:generate_pk_skmutates case inconsistentlyOnly the value used in
skis lower-cased, leavinglicenseTypeAbbreviationin the persisted item untouched.
If callers supply"SLP"the stored abbreviation will be mixed-case between the attribute value and the key, making ad-hoc debugging harder.Either normalise the attribute too or enforce lower-case earlier:
- license_type_abbr = in_data['licenseTypeAbbreviation'].lower() + in_data['licenseTypeAbbreviation'] = in_data['licenseTypeAbbreviation'].lower() + license_type_abbr = in_data['licenseTypeAbbreviation']backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
49-64: 👍 Renaming unused kwargs improves clarityReplacing
**kwargswith**_kwargsmakes the intent explicit and removes the need for Ruff suppressions.
No functional impact observed.backend/compact-connect/lambdas/python/common/common_test/test_constants.py (1)
1-96: Constants file looks good – single authoritative source improves test clarityCentralising default values eliminates magic literals scattered across tests and generators.
Nice addition!backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
1-15: Import organization looks good.The imports are well-organized with standard library imports first, followed by package-specific imports. The wildcard import of test constants is appropriately marked with a noqa comment to suppress linter warnings.
18-22: Class documentation is clear and helpful.The class docstring clearly explains the purpose of this utility class for generating test data with customizable options.
311-314: Well-documented private method.The method correctly documents that it's accessing private data that shouldn't be modified in runtime code. The noqa comment for the linter warning is appropriate.
316-398: Well-structured test data generation method.This comprehensive method effectively generates default provider detail responses with realistic test data. It properly handles both cases where provider records are provided or need to be generated with default values, and ensures consistent date formatting for testing.
47-68: Well-implemented adverse action data generator.The new
generate_default_adverse_actionmethod properly implements the data generation for the new adverse action feature, with appropriate fields and type conversions.
24-399: Overall well-designed test utility class.The TestDataGenerator class is well-designed with consistent patterns for generating and storing test data. It provides methods for all necessary data types including the new adverse action functionality, and allows for flexible customization through value overrides. This approach will significantly improve test maintainability compared to using static JSON files.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py
Show resolved
Hide resolved
...d/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
Show resolved
Hide resolved
...d/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
Outdated
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Outdated
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/adverse-actions
branch
from
da51e28 to
0eb4c1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
350-432:⚠️ Potential issueMissing adverse actions in default provider detail response.
While the class provides methods to generate adverse action data, the
generate_default_provider_detail_responsemethod doesn't include adverse actions in the default items list (lines 414-422). According to the PR objectives, provider responses should include adverse actions.Apply this diff to include adverse actions in the default response:
default_home_jurisdiction = TestDataGenerator.generate_default_home_jurisdiction_selection() TestDataGenerator._override_date_of_update_for_record( default_home_jurisdiction, datetime.fromisoformat(DEFAULT_HOME_UPDATE_DATE) ) + default_adverse_action = TestDataGenerator.generate_default_adverse_action() + TestDataGenerator._override_date_of_update_for_record( + default_adverse_action, datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP) + ) items = [ TestDataGenerator.generate_default_provider().to_dict(), default_license_record.to_dict(), default_license_update_record.to_dict(), default_privilege_record.to_dict(), default_privilege_update_record.to_dict(), default_military_affiliation.to_dict(), default_home_jurisdiction.to_dict(), + default_adverse_action.to_dict(), ]
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
78-109: Adverse action implementation looks good, previously fixed return statement.The implementation correctly generates and stores default adverse action records with appropriate customization options. The previously identified missing return statement has been properly addressed.
🧹 Nitpick comments (9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (1)
29-35: Return copies of mutable collections to keep the instance truly read-only
document_keys/file_namesexpose the internal list; callers can mutate the list and inadvertently modify_data.
Return a shallow copy (or a tuple) instead.- return self._data.get('documentKeys', []) + return list(self._data.get('documentKeys', []))Apply the same pattern for
file_names.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
123-124: Expose an immutable copy ofprivilege_jurisdictionsThe method currently returns the actual
setstored in_data, so callers can mutate the underlying record.- return self._data.get('privilegeJurisdictions', set()) + return set(self._data.get('privilegeJurisdictions', set()))backend/compact-connect/tests/app/test_api/test_provider_management_api.py (1)
475-734: High duplication – extract helper utilities to keep tests maintainableThe two new encumbrance endpoint tests copy ~150 lines of almost identical nested-resource traversal code.
Abstracting the traversal & assertions into helper methods (or parametrising the test) would:
- Make intent clearer (what varies vs. what is boilerplate)
- Reduce the chance of future copy-paste mistakes
- Shorten individual test methods, improving readability
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
608-656: Duplicate endpoint-registration code – favour a shared helper
_add_encumber_privilegeand_add_encumber_licensediffer only in:
- the parent resource variable
- the API model used
Extracting a private method
_add_encumber_endpoint(parent_resource, request_model)would reduce ~40 duplicated lines and make future updates safer.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
970-977: Unused helper method.This helper method for generating an adverse action item is defined but never used in the implementation. The encumber methods use
_generate_put_item_transactioninstead.Consider removing this unused method or refactoring the code to use it instead of the generic
_generate_put_item_transactionfor adverse actions.backend/compact-connect/docs/api-specification/latest-oas30.json (4)
414-504: Introduce license encumbrance endpoint.The new
POST /providers/{providerId}/licenses/jurisdiction/{jurisdiction}/licenseType/{licenseType}/encumbranceendpoint is well-defined with all necessary path parameters, request schema reference (SandboLicen62Uj6JPFczem), and Cognito admin authorizer scopes. Consider adding standard error responses (400,401,403) underresponsesto document validation and auth failures.
596-686: Introduce privilege encumbrance endpoint.The new
POST /providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/encumbranceendpoint mirrors the license encumbrance endpoint. Ensure both endpoints share consistent path formatting and error response definitions. Adding400,401, and403responses would improve spec completeness.
4254-4276: Define license encumbrance request schema.The
SandboLicen62Uj6JPFczemschema correctly lists the required fields for a license encumbrance. To enforce date validation, consider adding"format": "date"or apatternforencumbranceEffectiveDate.
5280-5302: Define privilege encumbrance request schema.The
SandboLicenFzKiEdNIK5oFschema is symmetric to the license version. Similarly, adding"format": "date"(or a regexpattern) toencumbranceEffectiveDatewill improve consistency and validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
backend/compact-connect/docs/api-specification/latest-oas30.json(6 hunks)backend/compact-connect/lambdas/python/common/cc_common/config.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(6 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/__init__.py(1 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(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py(5 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py(5 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/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json(1 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json(2 hunks)backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json(1 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/provider-data-v1/handlers/encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py(7 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(5 hunks)backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py(1 hunks)backend/compact-connect/tests/app/test_api/test_provider_management_api.py(17 hunks)backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_DEACTIVATION_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/ingest.py
- backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py
- backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/init.py
🚧 Files skipped from review as they are similar to previous changes (35)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.py
- backend/compact-connect/tests/resources/snapshots/PRIVILEGE_DEACTIVATION_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json
- backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/init.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
- backend/compact-connect/lambdas/python/common/cc_common/config.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py
- backend/compact-connect/stacks/api_stack/v1_api/api.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/privileges.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/common.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py
- backend/compact-connect/lambdas/python/common/common_test/test_constants.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCDataClass(15-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
ProviderRecordSchema(21-102)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCDataClass(15-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py (1)
MilitaryAffiliationRecordSchema(16-41)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
provider_id(21-22)compact(25-26)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (11)
AdverseActionData(9-126)compact(19-20)compact(23-24)provider_id(27-28)provider_id(31-32)jurisdiction(35-36)jurisdiction(39-40)license_type_abbreviation(43-44)license_type_abbreviation(47-48)license_type(51-52)license_type(55-56)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
ActiveInactiveStatus(180-182)CompactEligibilityStatus(185-187)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (10)
LicenseData(9-127)compact(26-27)compact(148-149)provider_id(22-23)provider_id(144-145)jurisdiction(30-31)jurisdiction(152-153)license_type(34-35)license_type(156-157)compact_eligibility(126-127)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (19)
PrivilegeData(12-121)compact(29-30)compact(33-34)compact(149-150)compact(153-154)provider_id(21-22)provider_id(25-26)provider_id(141-142)provider_id(145-146)jurisdiction(37-38)jurisdiction(41-42)jurisdiction(157-158)jurisdiction(161-162)administrator_set_status(109-110)administrator_set_status(113-114)license_type(53-54)license_type(57-58)license_type(165-166)license_type(169-170)backend/compact-connect/lambdas/python/common/cc_common/config.py (3)
provider_table(86-87)current_standard_datetime(247-251)dynamodb_client(36-37)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCNotFoundException(19-20)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (13)
backend/compact-connect/tests/app/test_api/test_provider_management_api.py (1)
553-566: Potential mismatch between looked-up and hard-coded logical IDsYou locate the resource via
find_resources(...)(👍) but then ignore that value and instead use
api_stack.get_logical_id(api_stack.api.v1_api.provider_management.encumbrance_privilege_resource …)
to buildencumbrance_resource_logical_id.
If the CFN logical ID ever changes (e.g., due to refactoring), the assertion thatlen(encumbrance_resource_logical_ids) == 1
will still pass, but the subsequenthas_resource_propertiescould reference the wrong ID and silently miss the bug.Consider re-using the ID returned from
find_resources:-encumbrance_resource_logical_id = api_stack.get_logical_id( - api_stack.api.v1_api.provider_management.encumbrance_privilege_resource.node.default_child -) +encumbrance_resource_logical_id = next(iter(encumbrance_resource_logical_ids))backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
14-24: Appropriate imports for new functionality.The added imports correctly bring in the necessary classes and enums for the new adverse action functionality. The
AdverseActionDataclass, status enums, and updated data model classes align with the PR's objective of supporting adverse actions for licenses and privileges.
924-946: Well-structured helper method for updating privilege status.This helper method encapsulates the transaction item generation for updating a privilege's administrator status, promoting code reuse and readability. The method handles all required DynamoDB formatting logic correctly.
947-969: Well-structured helper method for updating license eligibility status.Similar to the privilege status helper, this method correctly encapsulates the transaction item generation for updating a license's compact eligibility status, promoting code reuse and consistency.
978-985: Generic helper for transaction item generation.This helper method provides a generalized way to create transaction items, simplifying the code in the encumber methods.
backend/compact-connect/docs/api-specification/latest-oas30.json (2)
5-5: Confirm OpenAPI version update.The
versionfield has been updated to"2025-04-28T20:50:55Z". Ensure this matches the release date of this API spec and is reflected across CDK/API documentation.
1903-1905: Schema deactivationNote requirement added.The
deactivationNoteproperty is now marked as required inSandboLiceno4bc0FEXEiqd. This aligns with the deactivation flow requiring a note. Add tests to validate that missing this field results in a 400 response.backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (6)
1-16: Well-structured imports with appropriate organization.The imports are organized logically with standard library imports first, followed by application-specific imports. The noqa directive for star imports is correctly applied to suppress linting warnings for importing all constants.
18-22: Clean class definition with good documentation.The class documentation clearly explains the purpose of the TestDataGenerator utility class.
24-28: Helper method enhances API response testing.This utility method correctly converts data class instances into API response format using ResponseEncoder, ensuring test data matches production format.
29-59: Well-documented API event generator with flexible overrides.The method provides a clean approach to generating test API events with separate controls for authentication context (sub, scope) and general payload overrides. The documentation clearly explains the parameters and purpose.
262-266: Useful configuration accessor for license type abbreviations.This helper method provides a clean interface to retrieve license type abbreviations from configuration, supporting the new adverse action functionality.
344-348: Appropriate handling of date overrides for testing.The method correctly handles the special case of overriding dateOfUpdate fields on data class instances, which are typically immutable at runtime. The noqa comment properly justifies accessing a protected attribute.
...t-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1079-1087:⚠️ Potential issueIncorrect docstring in encumber_license method.
The docstring incorrectly refers to updating a privilege record instead of a license record, and describes the wrong status change. License encumbrance updates the
compactEligibilitytoineligible, not theadministratorSetStatustoinactive.Apply this fix:
def encumber_license(self, adverse_action: AdverseActionData) -> None: """ Adds an adverse action record for a privilege for a provider in a jurisdiction. - This will update the privilege record to have a administratorSetStatus of 'inactive'. + This will update the license record to have a compactEligibility of 'ineligible'. :param AdverseActionData adverse_action: The details of the adverse action to be added to the records - :raises CCNotFoundException: If the privilege record is not found + :raises CCNotFoundException: If the license record is not found """backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (1)
573-607:⚠️ Potential issueMissing encryption key grant for encumbrance handler.
The encumbrance handler needs access to decrypt the provider data table's encryption key, like other handler implementations such as
_get_provider_handlerand_query_providers_handler. Without this grant, the Lambda function may encounter access denied errors when reading encrypted attributes.Apply this fix:
def _add_provider_encumbrance_handler( self, provider_data_table: ProviderTable, staff_users_table: Table, event_bus: EventBus, lambda_environment: dict, ) -> PythonFunction: """Create and configure the Lambda handler for encumbering a provider's privilege or license.""" encumbrance_handler = PythonFunction( self.resource, 'ProviderEncumbranceHandler', description='Provider encumbrance handler', lambda_dir='provider-data-v1', index=os.path.join('handlers', 'encumbrance.py'), handler='encumbrance_handler', environment=lambda_environment, alarm_topic=self.api.alarm_topic, ) provider_data_table.grant_read_write_data(encumbrance_handler) + self.api.shared_encryption_key.grant_decrypt(encumbrance_handler) staff_users_table.grant_read_data(encumbrance_handler) event_bus.grant_put_events_to(encumbrance_handler)Make sure to pass
shared_encryption_key(or the correct key reference) when calling this helper method.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
986-994: Consider adding a return type annotation.The method signature doesn't specify a return type, although the implementation doesn't return anything. Adding a return type annotation would improve code clarity.
-def encumber_privilege(self, adverse_action: AdverseActionData) -> None: +def encumber_privilege(self, adverse_action: AdverseActionData) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_management.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (7)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
provider_table(86-87)backend/compact-connect/stacks/persistent_stack/provider_table.py (1)
ProviderTable(8-80)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/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(28-36)backend/compact-connect/stacks/api_stack/cc_api.py (1)
parameter_body_validator(262-263)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (3)
post_privilege_encumbrance_request_model(381-411)message_response_model(21-39)post_license_encumbrance_request_model(414-444)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (12)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (7)
924-946: Well-structured helper method for privilege status updates.This helper method effectively abstracts the creation of DynamoDB transaction items for updating privilege statuses, promoting code reuse and maintainability.
947-969: Well-structured helper method for license eligibility updates.This helper method effectively abstracts the creation of DynamoDB transaction items for updating license compact eligibility status, promoting code reuse and maintainability.
970-977: Clean implementation of adverse action item generator.The method effectively encapsulates the logic for creating a DynamoDB transaction item to add an adverse action record, promoting reusability.
978-985: Good generic transaction item generator.This helper abstracts the common pattern of serializing and putting items in DynamoDB transactions, reducing code duplication across the other transaction item generators.
1005-1013: Comprehensive privilege record retrieval with error handling.The code properly handles the case when a privilege record is not found, providing a detailed error message that includes the jurisdiction.
1019-1027: Smart logic to avoid unnecessary updates.The code checks if the privilege is already inactive before attempting to update its status, which prevents unnecessary write operations and improves efficiency.
1051-1075: Robust atomic transaction implementation.The code uses DynamoDB transactions to ensure that all database operations (adding adverse action, creating history record, and updating the privilege status) are performed atomically, maintaining data consistency even in case of failures.
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (5)
34-37: Renamed class to better reflect expanded responsibilities.Renamed from
QueryProviderstoProviderManagementto better represent the class's broadened scope, which now includes provider encumbrance functionality.
75-89: Well-structured resource hierarchy for privileges and licenses.Creating these nested resources at the instance level improves organization and makes the code more maintainable, especially with the addition of new endpoints like encumbrance.
120-129: Clear initialization of encumbrance functionality.The encumbrance handler is properly initialized and stored as an instance variable, and the methods to add encumbrance endpoints are called in a logical order.
608-633: Complete API endpoint configuration for privilege encumbrance.The privilege encumbrance endpoint is properly configured with appropriate request validator, request models, response models, and authorization. The integration with the encumbrance handler is cleanly implemented.
634-656: Complete API endpoint configuration for license encumbrance.The license encumbrance endpoint is properly configured with appropriate request validator, request models, response models, and authorization. The integration with the encumbrance handler is cleanly implemented.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
93-96: Hard-coded'slp'abbreviation – see earlier discussionThe literal
'slp'is intentionally left to “fail loud” when core test constants change, per maintainer feedback.
No further action needed.
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
313-317: Variable mis-named after refactor
loaded_privilege_dataactually holds aLicenseDatainstance. Renaming improves readability and avoids confusion when grepping tests.- loaded_privilege_data = LicenseData.from_database_record(item) + loaded_license_data = LicenseData.from_database_record(item) ... - loaded_privilege_data.to_dict(), + loaded_license_data.to_dict(),backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
970-977: Potentially unnecessary helper method.This helper method is only used once in the current implementation. Consider whether it provides enough value to justify its existence as a separate method compared to using the more generic
_generate_put_item_transaction.- def _generate_put_adverse_action_item(self, adverse_action: AdverseActionData): - return { - 'Put': { - 'TableName': self.config.provider_table.name, - 'Item': TypeSerializer().serialize(adverse_action.serialize_to_database_record())['M'], - }, - }
986-1078: Comprehensive implementation of privilege encumbrance with return type mismatch.The
encumber_privilegemethod correctly implements:
- Finding the privilege record
- Checking if it's already inactive (avoiding unnecessary updates)
- Creating an update record for audit history
- Performing a transactional write to ensure data consistency
However, the method has no return type annotation but also doesn't return any value (implicitly returns None).
- def encumber_privilege(self, adverse_action: AdverseActionData) -> None: + def encumber_privilege(self, adverse_action: AdverseActionData) -> None:And update the docstring to clarify the return expectation:
""" Adds an adverse action record for a privilege for a provider in a jurisdiction. This will update the privilege record to have a administratorSetStatus of 'inactive', and add a privilege update record to show the encumbrance event. :param AdverseActionData adverse_action: The details of the adverse action to be added to the records :raises CCNotFoundException: If the privilege record is not found + :return: None """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.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/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 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(1 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(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/init.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/init.py
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/init.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
924-946: Well-structured helper method for privilege status updates.This method nicely encapsulates the DynamoDB transaction item generation for updating a privilege's administrator set status, making the transaction creation more maintainable and readable.
947-969: Well-structured helper method for license eligibility updates.This helper method cleanly abstracts the creation of a DynamoDB transaction item for updating a license's compact eligibility status.
978-985: Good abstraction for DynamoDB put operations.This generic helper method simplifies the creation of put transactions and is reused effectively throughout the new code.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
125-199: 🛠️ Refactor suggestionPrivilegeUpdateData is missing required construction flag.
According to a previous review, this class should not be instantiated empty, but it's missing the flag that enforces this constraint.
# Define the record schema at the class level _record_schema = PrivilegeUpdateRecordSchema() +# Prevent empty instantiation since this class represents a concrete update event +_requires_data_at_construction = True
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
110-115: 🛠️ Refactor suggestionPotential serialization issue with enum value in administrator_set_status setter.
The property returns an
ActiveInactiveStatusenum, but the setter doesn't handle conversion when storing the value, which might cause serialization issues when the enum instance is stored directly.@administrator_set_status.setter def administrator_set_status(self, value: ActiveInactiveStatus) -> None: - self._data['administratorSetStatus'] = value + # Store the enum's value string, not the enum instance itself + self._data['administratorSetStatus'] = value.value if isinstance(value, ActiveInactiveStatus) else value
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
970-977: Unused helper method.This method is defined but never used in the implementation. The code instead uses
_generate_put_item_transactionwithadverse_action.serialize_to_database_record()in both encumbrance methods.Either remove this unused method or refactor the encumbrance methods to use it:
- # Add the adverse action record for the privilege - self._generate_put_item_transaction(adverse_action.serialize_to_database_record()), + # Add the adverse action record for the privilege + self._generate_put_adverse_action_item(adverse_action),
1153-1154: Comment references privilege record instead of license record.There's an inconsistency in the comment which references "privilege record's administratorSetStatus" when describing a license record update.
- # Set the privilege record's administratorSetStatus to inactive and update the dateOfUpdate + # Set the license record's compactEligibility to ineligible and update the dateOfUpdate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
924-946: Well-structured utility method for privilege status updates.This helper method nicely encapsulates the creation of transaction items for updating privilege status, improving code reusability across both new encumbrance methods.
947-969: Well-structured utility method for license eligibility updates.The method properly abstracts the creation of transaction items for updating license compact eligibility status, maintaining consistency with the privilege counterpart.
978-985: Generic transaction item helper is well implemented.This utility method abstracts the common pattern of creating a "Put" transaction item, reducing code duplication across the encumbrance methods.
986-1077: Comprehensive implementation of privilege encumbrance.The method correctly implements the following workflow:
- Finds the privilege record
- Checks if it's already inactive (avoiding unnecessary updates)
- Creates an update record for audit history
- Performs a transactional write to ensure data consistency
The implementation properly follows the learned pattern for CCDataClass instantiation using
from_database_record()rather than direct constructor instantiation.
1078-1167: License encumbrance implementation is complete and correct.The method properly:
- Fetches the license record
- Verifies if it's already ineligible
- Creates a license update record for history/audit
- Executes all changes in a single transaction for data consistency
The implementation follows a consistent pattern with the privilege encumbrance method, which makes the code more maintainable.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
11-123: Well-structured PrivilegeData class with properly typed properties.The implementation provides clean accessor methods with appropriate type hints. The class correctly uses the CCDataClass pattern for serialization and deserialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
966-973: Unused helper method.The
_generate_put_adverse_action_itemmethod is defined but never used. The encumbrance methods use_generate_put_item_transactionwithadverse_action.serialize_to_database_record()instead.Consider either:
- Using this specific method in the encumbrance methods, or
- Removing this unused method to avoid confusion
- def _generate_put_adverse_action_item(self, adverse_action: AdverseActionData): - return { - 'Put': { - 'TableName': self.config.provider_table.name, - 'Item': TypeSerializer().serialize(adverse_action.serialize_to_database_record())['M'], - }, - }
1071-1157: Solid implementation of license encumbrance with minor documentation issue.The implementation correctly handles license encumbrance similar to privilege encumbrance, following the same patterns for consistency.
However, there's an incorrect comment at line 1146-1147:
- # Set the privilege record's administratorSetStatus to inactive and update the dateOfUpdate + # Set the license record's compactEligibility to ineligible and update the dateOfUpdate
1098-1099: Use f-strings consistently for error messages.The error message concatenation style is inconsistent with the rest of the codebase.
- raise CCNotFoundException(f'{message} {adverse_action.jurisdiction}') from e + raise CCNotFoundException(f'License not found for jurisdiction {adverse_action.jurisdiction}') from eThis matches the style used in the privilege method at line 1010.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/init.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
924-981: Well-designed helper methods for DynamoDB transaction items.These helper methods effectively abstract DynamoDB transaction construction, making the encumbrance methods cleaner and more maintainable. They follow a consistent pattern and handle serialization appropriately.
982-1070: Robust implementation of privilege encumbrance.The method correctly implements:
- Privilege record retrieval
- Status check to avoid unnecessary updates
- Creation of an audit update record
- Transactional writes for atomicity
The error handling, logging, and context management follow best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1063-1149: Fix comments in the license encumbrance method.The implementation is correct, but several comments still incorrectly refer to privileges instead of licenses.
Apply these changes to fix the comments:
-# Update the privilege record and create history record +# Update the license record and create history record logger.info('Encumbering license') -# we add the adverse action record for the license, -# the license update record, and update the license record to ineligible if it is not already ineligible +# we add the adverse action record for the license, +# the license update record, and update the license record to ineligible if it is not already ineligible transact_items = [ # Create a history record, reflecting this change self._generate_put_transaction_item(license_update_record), - # Add the adverse action record for the privilege + # Add the adverse action record for the license self._generate_put_transaction_item(adverse_action.serialize_to_database_record()), ]
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
924-944: Well-designed utility method for generating privilege status update transactions.This helper method cleanly abstracts the process of creating DynamoDB transaction items for updating privilege status, promoting code reuse and consistency.
945-965: Good abstraction for license compact eligibility status updates.Similar to the privilege helper method, this provides a clean abstraction for license-specific status updates, improving code maintainability.
966-973: Clean utility method to reduce transaction boilerplate.This simple helper method effectively reduces code duplication across the transaction generation process.
974-1062: Comprehensive implementation of privilege encumbrance.The method correctly:
- Retrieves the privilege record
- Checks if it's already inactive (avoiding unnecessary updates)
- Creates an update record for audit history
- Performs a transactional write to ensure data consistency
The context manager for logging is a good practice that ensures consistent log context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
18-18: Remove unused importLicenseUpdateRecordSchema.The
LicenseUpdateRecordSchemais imported but never used in the code. The implementation usesLicenseUpdateData.create_new()instead at line 1107.-from cc_common.data_model.schema.license import LicenseData, LicenseUpdateRecordSchema, LicenseUpdateData +from cc_common.data_model.schema.license import LicenseData, LicenseUpdateData🧰 Tools
🪛 Ruff (0.8.2)
18-18:
cc_common.data_model.schema.license.LicenseUpdateRecordSchemaimported but unusedRemove unused import:
cc_common.data_model.schema.license.LicenseUpdateRecordSchema(F401)
1063-1072: Add return type annotation to method signature for consistency.The method docstring should explicitly specify the return type as
Noneto be consistent with theencumber_privilegemethod.-def encumber_license(self, adverse_action: AdverseActionData) -> None: +def encumber_license(self, adverse_action: AdverseActionData) -> None: """ Adds an adverse action record for a license for a provider in a jurisdiction. This will update the license record to have a compactEligibility of 'ineligible', and add a license update record to show the encumbrance event. :param AdverseActionData adverse_action: The details of the adverse action to be added to the records :raises CCNotFoundException: If the license record is not found + :return: None """
📜 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(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
18-18: cc_common.data_model.schema.license.LicenseUpdateRecordSchema imported but unused
Remove unused import: cc_common.data_model.schema.license.LicenseUpdateRecordSchema
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
924-944: Well-designed helper method for privilege status updates.This helper method effectively abstracts the DynamoDB update operation for privilege administrator status, following the DRY principle and improving code maintainability.
945-965: Well-designed helper method for license eligibility updates.This helper method effectively abstracts the DynamoDB update operation for license compact eligibility status, consistent with the privilege helper method structure.
966-972: Clean implementation of a reusable transaction item generator.This small helper method reduces code duplication in the encumbrance methods and improves readability by abstracting the DynamoDB Put operation creation.
974-1062: Comprehensive implementation of privilege encumbrance with atomic operations.The
encumber_privilegemethod successfully implements the encumbrance workflow:
- Retrieves the privilege record
- Intelligently checks if it's already inactive to avoid unnecessary updates
- Creates an update record to maintain audit history
- Performs a transactional write to ensure data consistency
The implementation properly uses the context manager for logging and follows the established patterns for the codebase.
1073-1149: Comprehensive implementation of license encumbrance with atomic operations.The
encumber_licensemethod successfully implements the encumbrance workflow:
- Retrieves the license record
- Intelligently checks if it's already ineligible to avoid unnecessary updates
- Creates an update record to maintain audit history
- Performs a transactional write to ensure data consistency
The implementation properly uses the context manager for logging and follows the established patterns for the codebase.
|
@jusdino I've wrapped up all the code rabbit related fixes, so this should now be fully ready for a human review, thanks. |
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
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
Outdated
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/provider/api.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
292-320: Fix method name to accurately reflect test purposeThe method name
test_license_encumbrance_handler_sets_privilege_record_to_inactive_in_provider_data_tableis inaccurate. This test is verifying the license record status change, not the privilege record.Rename the method to better reflect its purpose:
-def test_license_encumbrance_handler_sets_privilege_record_to_inactive_in_provider_data_table(self): +def test_license_encumbrance_handler_sets_license_record_to_ineligible_in_provider_data_table(self):Also, line 316 should use a more accurately named variable:
-loaded_privilege_data = LicenseData.from_database_record(item) +loaded_license_data = LicenseData.from_database_record(item)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(5 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py(5 hunks)backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json(1 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/provider-data-v1/handlers/encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(3 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)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/compact-connect/lambdas/python/common/tests/resources/api/adverse-action-post.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/init.py
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py
🧰 Additional context used
🪛 Biome (1.9.4)
backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
[error] 18-18: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
[error] 18-18: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (6)
38-39: Property patching approach looks goodThe direct patching of
cc_common.config._Config.current_standard_datetimewith a datetime value is correct since it's a property, not a method. This ensures consistent timestamps during tests.
94-95: Hardcoded license type abbreviation is intentionalThe hardcoded
'slp'abbreviation is intentional according to previous comments. This design ensures that if test constants change, tests will deliberately fail, requiring conscious updates by developers.
129-130: Hardcoded license type abbreviation is intentionalThe hardcoded
'slp'abbreviation is intentional according to previous comments. This design ensures that if test constants change, tests will deliberately fail, requiring conscious updates by developers.
237-238: Hardcoded license type abbreviation is intentionalThe hardcoded
'slp'abbreviation is intentional according to previous comments. This design ensures that if test constants change, tests will deliberately fail, requiring conscious updates by developers.
273-274: Hardcoded license type abbreviation is intentionalThe hardcoded
'slp'abbreviation is intentional according to previous comments. This design ensures that if test constants change, tests will deliberately fail, requiring conscious updates by developers.
1-321: Comprehensive test coverage for encumbrance functionalityThe test suite thoroughly covers the critical functionality of the new encumbrance endpoints, testing both API responses and database state changes. It validates the creation of adverse action records, the updates to privileges/licenses, and the status changes that occur during encumbrance.
The test structure follows best practices with clear setup, execution, and verification steps.
backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (1)
25-51: Consider using direct dictionary access for required propertiesSince the class requires data at construction, required properties like
providerIdandcompactshould use direct dictionary access rather than.get(), which is typically used for optional values. Using direct access would fail fast with a clearKeyErrorif required data is missing.@property def providerId(self) -> UUID: - return self._data.get('providerId') + return self._data['providerId'] @property def compact(self) -> str: - return self._data.get('compact') + return self._data['compact']While
.get()is appropriate for optional properties (like the collections with default values), using direct access for required fields provides stronger guarantees and clearer errors.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
293-293: Method name is inconsistent with what it's actually testing.The method name
test_license_encumbrance_handler_sets_privilege_record_to_inactive_in_provider_data_tablesuggests it's testing a privilege record, but it's actually testing a license record.Consider renaming the method to correctly reflect what it's testing:
-def test_license_encumbrance_handler_sets_privilege_record_to_inactive_in_provider_data_table(self): +def test_license_encumbrance_handler_sets_license_record_to_ineligible_in_provider_data_table(self):
316-316: Variable name is inconsistent with the type of data it contains.The variable name
loaded_privilege_datais misleading as it's actually holding license data, not privilege data.- loaded_privilege_data = LicenseData.from_database_record(item) + loaded_license_data = LicenseData.from_database_record(item)Also update the assertion to use the renamed variable:
self.assertEqual( expected_license_data.to_dict(), - loaded_privilege_data.to_dict(), + loaded_license_data.to_dict(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.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/record.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 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(1 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(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py(7 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)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/init.py
- backend/compact-connect/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/init.py
- backend/compact-connect/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
🧰 Additional context used
🧠 Learnings (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.831Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.089Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
🪛 Ruff (0.8.2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
5-5: marshmallow.fields.Boolean imported but unused
Remove unused import: marshmallow.fields.Boolean
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (1)
22-23: Good implementation of empty construction preventionSetting
_requires_data_at_construction = Trueensures that the class will fail fast with a clear error message when initialization is attempted without providing valid data. This is appropriate for a read-only data class that doesn't provide setters.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
15-140: Excellent base class implementation for data modelsThe
CCDataClassimplementation provides a robust foundation for data models with clear factory methods, validation, and serialization utilities. The documentation is comprehensive, and the code handles edge cases like empty data and validation errors well.Particularly good design points:
- Factory method pattern with
create_newandfrom_database_record- Constructor-level validation to prevent misuse
- Configurable validation of empty initialization via
_requires_data_at_construction- Proper deepcopy usage to prevent mutation of the internal data
- Clear separation between database serialization and dict representation
210-226: Well-defined enums for adverse action functionalityThe addition of
AdverseActionAgainstEnumand extension ofUpdateCategorywith new values likeENCUMBRANCEprovides a clean type-safe way to model the different entities that can have adverse actions and the types of updates. This design enables static type checking and IDE auto-completion, improving developer experience.
243-254: Comprehensive enum for clinical privilege action categoriesThe
ClinicalPrivilegeActionCategoryenum provides a well-documented set of values based on NPDB standards. Including the source reference in the docstring is excellent practice. The enum values are descriptive and match real-world categories.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (5)
94-94: Hardcoded license type abbreviation 'slp' in query.Following project conventions discussed in previous reviews, the hardcoded 'slp' value is deliberately used instead of a dynamic lookup to ensure tests fail if the default test data changes.
38-38: Correctly patching the datetime property.This patch correctly supplies a direct datetime value rather than a lambda because
current_standard_datetimeis implemented as a property in the CompactConnect codebase.
1-26: Well-structured test setup with clear constants.The import section and constants are well-organized, making the test setup clear and maintainable. Good job defining the endpoint resources as constants for reuse.
42-64: Well-designed test helper for test data preparation.The
_when_testing_valid_privilege_encumbrancehelper method effectively encapsulates test setup logic, making the test cases more readable and maintainable.
28-35: Good factoring of test body generation.Extracting the test body generation into a separate function improves reusability and readability by avoiding duplication between privilege and license test cases.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py
Show resolved
Hide resolved
...d/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
Outdated
Show resolved
Hide resolved
...d/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py (1)
37-62: Critical test for compact eligibility with encumbered licenses.This test properly verifies the business rule that providers with any encumbered license should be ineligible for compact privileges. The test clearly demonstrates how the
PROVIDER_ENCUMBEREDstatus impacts compact eligibility calculations.However, I recommend adding additional test cases to cover other encumbrance statuses (
ENCUMBERED,UNENCUMBERED) to ensure complete coverage of the eligibility determination logic.Consider adding these additional test cases:
def test_compact_eligibility_with_different_encumbrance_statuses(self): from cc_common.data_model.schema.base_record import CalculatedStatusRecordSchema from cc_common.data_model.schema.common import ( ActiveInactiveStatus, CompactEligibilityStatus, LicenseEncumberedStatusEnum, ) schema = CalculatedStatusRecordSchema() base_provider = { 'jurisdictionUploadedLicenseStatus': ActiveInactiveStatus.ACTIVE, 'jurisdictionUploadedCompactEligibility': CompactEligibilityStatus.ELIGIBLE, 'dateOfExpiration': '2050-01-01', 'pk': 'COMPACT#AZ#PROVIDER#1234567890', 'sk': 'COMPACT#AZ#PROVIDER#1234567890', 'type': 'provider', 'dateOfUpdate': '2025-01-01T00:00:00Z', } # Test with directly encumbered license provider_encumbered = {**base_provider, 'encumberedStatus': LicenseEncumberedStatusEnum.ENCUMBERED} result = schema.load(provider_encumbered) self.assertEqual(CompactEligibilityStatus.INELIGIBLE.value, result['compactEligibility']) # Test with unencumbered license provider_unencumbered = {**base_provider, 'encumberedStatus': LicenseEncumberedStatusEnum.UNENCUMBERED} result = schema.load(provider_unencumbered) self.assertEqual(CompactEligibilityStatus.ELIGIBLE.value, result['compactEligibility']) # Test with no encumbrance status (should default to UNENCUMBERED) provider_no_encumbrance = {**base_provider} result = schema.load(provider_no_encumbrance) self.assertEqual(CompactEligibilityStatus.ELIGIBLE.value, result['compactEligibility'])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py(7 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.831Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
🧬 Code Graph Analysis (2)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (1)
PrivilegeRecordSchema(53-143)
backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py (3)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-51)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
CalculatedStatusRecordSchema(91-151)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
ActiveInactiveStatus(228-230)CompactEligibilityStatus(233-235)LicenseEncumberedStatusEnum(238-244)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (9)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (6)
15-141: Excellent implementation of the base data class abstraction.The
CCDataClassprovides a clean abstraction layer between data models and database schemas, with well-defined factory methods that enforce proper instantiation patterns. The separation of concerns between data validation, serialization, and database interaction is well-implemented.The prevention of direct instantiation through an internal flag and comprehensive docstrings make this a maintainable foundation for your data layer.
82-85: Improved guard against incorrect instantiation.The check for database keys in the data is now correctly implemented using membership check (
in) rather than truthiness evaluation. This properly prevents falsy values like empty strings or None from bypassing the guard.
210-217: Well-defined enum for adverse action targets.The
AdverseActionAgainstEnumcleanly defines the scope of records that can be affected by adverse actions, making it easy to extend in the future if additional record types need to be supported.
225-226: Clear distinction of "encumbrance" as a separate update category.Adding
ENCUMBRANCEto theUpdateCategoryenum creates a clear separation between encumbrance updates and other types of updates, which will help with tracking and event handling of adverse actions.
238-252: Comprehensive encumbrance status modeling for both licenses and privileges.The
LicenseEncumberedStatusEnumandPrivilegeEncumberedStatusEnumprovide a complete model for representing encumbrance states, with well-documented special statuses:
PROVIDER_ENCUMBERED- captures the cascading effect where a provider's other licenses are marked when any license is encumberedLICENSE_ENCUMBERED- handles the case where a privilege is indirectly affected by an encumbered licenseThis modeling enables accurate propagation of encumbrance effects throughout the provider's profile.
259-270: Properly sourced clinical privilege action categories.The
ClinicalPrivilegeActionCategoryenum correctly implements standardized NPDB categories with the source citation, ensuring alignment with industry standards for adverse action reporting.backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py (3)
100-111: Good coverage for encumbered privilege status logic.This test correctly verifies that privileges with an
encumberedstatus are marked inactive regardless of expiration date, which is a critical aspect of the encumbrance feature.
112-123: Complete test coverage with explicit unencumbered case.This test complements the previous one by explicitly verifying that privileges with an
unencumberedstatus remain active (when not expired), ensuring the positive case is also covered.
124-135: Proper default behavior testing for backward compatibility.This test ensures that existing records without an
encumberedStatusfield default to active status, maintaining backward compatibility with the existing data model. This is crucial for a smooth rollout of the encumbrance feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
229-229: Remove whitespace from blank line.There's unnecessary whitespace on this blank line, which is flagged by static analysis tools.
- +🧰 Tools
🪛 Ruff (0.8.2)
229-229: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/utils.py(1 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/compact-connect/lambdas/python/common/cc_common/utils.py
229-229: Blank line contains whitespace
Remove whitespace from blank line
(W293)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
231-268: Well-implemented decorator for state-level authorization.The new
authorize_state_level_only_actiondecorator is correctly implemented and follows the same pattern as existing authorization decorators. It properly extractscompactandjurisdictionfrom path parameters, verifies the caller's scopes, and raises appropriate exceptions for missing parameters or unauthorized access.This addition supports the new provider encumbrance API endpoints that require state-level authorization to perform adverse action encumbrance on privileges and licenses.
landonshumway-ia
force-pushed
the
feat/adverse-actions
branch
from
5dff541 to
05fc706
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1091-1213: 🛠️ Refactor suggestionInconsistent update type in the license encumbrance method.
While the
encumber_privilegemethod uses theUpdateCategory.ENCUMBRANCEenum, theencumber_licensemethod uses a string literal 'encumbrance'. This inconsistency could lead to issues if the enum values change.Additionally, the return value is missing from the docstring.
def encumber_license(self, adverse_action: AdverseActionData) -> None: """ Adds an adverse action record for a license for a provider in a jurisdiction. This will update the license record to have a compactEligibility of 'ineligible', and add a license update record to show the encumbrance event. :param AdverseActionData adverse_action: The details of the adverse action to be added to the records :raises CCNotFoundException: If the license record is not found + :return: None """ # ...code continues... license_update_record = LicenseUpdateData.create_new( { 'type': 'licenseUpdate', - 'updateType': 'encumbrance', + 'updateType': UpdateCategory.ENCUMBRANCE, 'providerId': adverse_action.providerId, # ...rest of the code...
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1002-1090: Well-implemented privilege encumbrance functionality.The implementation correctly:
- Retrieves the privilege record
- Checks if it's already encumbered
- Creates an update record
- Performs a transactional write
The return value is missing from the docstring. Consider adding
:return: Noneto complete the documentation.def encumber_privilege(self, adverse_action: AdverseActionData) -> None: """ Adds an adverse action record for a privilege for a provider in a jurisdiction. This will update the privilege record to have a administratorSetStatus of 'inactive', and add a privilege update record to show the encumbrance event. :param AdverseActionData adverse_action: The details of the adverse action to be added to the records :raises CCNotFoundException: If the privilege record is not found + :return: None """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/compact-connect/lambdas/python/common/cc_common/utils.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (8)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
TstFunction(22-394)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
ClinicalPrivilegeActionCategory(259-269)PrivilegeEncumberedStatusEnum(247-251)LicenseEncumberedStatusEnum(238-244)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (10)
put_default_privilege_record_in_provider_table(252-258)generate_test_api_event(30-58)generate_default_adverse_action(79-98)generate_default_privilege_update(267-292)generate_default_privilege(221-243)put_default_provider_record_in_provider_table(329-341)put_default_license_record_in_provider_table(186-192)generate_default_license_update(195-218)generate_default_license(149-183)generate_default_provider(295-326)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (11)
jurisdiction(37-38)jurisdiction(165-166)compact(33-34)compact(161-162)providerId(29-30)providerId(157-158)licenseType(41-42)licenseType(169-170)encumberedStatus(133-134)LicenseUpdateData(137-182)LicenseData(10-134)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (20)
jurisdiction(40-41)jurisdiction(44-45)jurisdiction(170-171)jurisdiction(174-175)compact(32-33)compact(36-37)compact(162-163)compact(166-167)providerId(24-25)providerId(28-29)providerId(154-155)providerId(158-159)licenseType(56-57)licenseType(60-61)licenseType(178-179)licenseType(182-183)PrivilegeUpdateData(137-210)PrivilegeData(13-134)encumberedStatus(122-123)encumberedStatus(126-127)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.py (9)
jurisdiction(38-39)jurisdiction(42-43)compact(22-23)compact(26-27)providerId(30-31)providerId(34-35)licenseType(54-55)licenseType(58-59)AdverseActionData(9-121)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (4)
compact(28-29)providerId(24-25)encumberedStatus(130-131)ProviderData(12-131)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (1)
encumbrance_handler(28-36)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (15)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
12-22: Well-designed class constructor requirements.The
ProviderDataclass properly enforces the presence of valid data at construction time with_requires_data_at_construction = True. This approach prevents potentialKeyErrorexceptions when accessing required properties later.
129-132: LGTM: Well-implemented encumbered status property for providers.The
encumberedStatusproperty implementation is clean and correctly handles the optional nature of the field by using.get()to returnNonewhen not present. This addition is important for supporting the new adverse actions functionality.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (13)
1-27: LGTM: Proper imports and constants definition.The imports are well organized and the constants for endpoint resources are clearly defined. The test data constants are properly imported from the test_constants module.
28-35: Good use of shared test data generation.The
_generate_test_bodyhelper function provides consistent test body data across tests. This promotes maintainability as any changes to the required request body structure only need to be updated in one place.
37-41: Proper mocking setup for deterministic tests.The test class correctly uses
@mock_awsfrom moto and patches the datetime property. As discussed in previous feedback, the datetime property is correctly patched with a direct value rather than a lambda sincecurrent_standard_datetimeis implemented as a property, not a method.
42-64: LGTM: Well-implemented test setup helper.The
_when_testing_valid_privilege_encumbrancehelper method creates a realistic test environment with a privilege record and generates a properly structured API event with appropriate scopes, path parameters and request body. This promotes test readability and reduces duplication.
65-78: LGTM: Clean basic functionality test.This test properly verifies the happy path for privilege encumbrance, checking that a valid request returns a 200 status code and the expected success message.
94-95: Hardcoded license type abbreviation in query is intentional.The hardcoded
'slp'in the key condition is deliberate. As noted in previous feedback, this approach ensures the test will fail if the default license type changes, requiring developers to consciously update related tests.
79-113: Comprehensive verification of adverse action record creation.The test thoroughly verifies that an adverse action record is properly created in the provider data table with the expected fields. It uses the TestDataGenerator to create a reference adverse action for comparison, which helps ensure consistent expectations across tests.
114-147: Good verification of privilege update record creation.The test appropriately verifies that a privilege update record is created with the expected update type and values. This ensures the system maintains a proper audit trail of encumbrance actions.
148-180: Thorough verification of privilege record update.The test correctly verifies that the privilege record itself is updated with the encumbered status, ensuring the handler not only creates records but also updates the original privilege's state.
181-196: Important authorization test.The test correctly verifies that compact admins cannot perform encumbrance actions, which should be restricted to state admins. This is an important security check to ensure proper access control.
198-227: Well-structured license encumbrance test setup.Similar to the privilege encumbrance tests, this test class uses a helper method to set up the test environment, including creating both a provider record and a license record. This setup correctly mimics the production environment where licenses are associated with providers.
343-372: Important provider-level encumbrance test.This test verifies a critical aspect of the encumbrance system - that when a license is encumbered, the provider record is also updated with an encumbered status. This cascading update is important for the overall encumbrance system to work correctly.
373-389: Clear authorization boundary testing.Like the privilege encumbrance tests, this test verifies that compact admins cannot encumber licenses. The descriptive comment "Verifying that only state admins are allowed to encumber licenses" adds helpful context to the test.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/adverse-actions
branch
from
05fc706 to
c7c003b
Compare
landonshumway-ia
force-pushed
the
feat/adverse-actions
branch
from
6fbaa64 to
5936647
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
136-144:⚠️ Potential issueMissing _requires_data_at_construction flag for PrivilegeUpdateData
The
PrivilegeUpdateDataclass docstring says it "requires valid data when created - it cannot be instantiated empty and populated later" (lines 139-140), but the class doesn't set the_requires_data_at_construction = Trueflag to enforce this constraint. This is inconsistent with the documented behavior.# Define the record schema at the class level _record_schema = PrivilegeUpdateRecordSchema() +# Disallow empty construction – this class represents a concrete update event +_requires_data_at_construction = TrueWithout this flag,
create_new()will happily return an empty instance, contrary to the note in the docstring.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
111-113: Correction: Return type annotation should be enum, not stringThe
administratorSetStatusproperty getter returns the raw value from the data dictionary, but the type annotation indicates it returns a string. However, based on the setter implementation (which correctly handles both enum instances and string values), the getter should return an enum instance to be consistent with the typing contract.@property -def administratorSetStatus(self) -> str: +def administratorSetStatus(self) -> ActiveInactiveStatus: return self._data['administratorSetStatus']This would make the getter's return type consistent with the setter's parameter type, and matches the pattern seen in other enums in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py(7 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(4 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/utils.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py(1 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/compact-connect/lambdas/python/common/cc_common/utils.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.831Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (13)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
116-119: Well-implemented setter ensures enum value serializationThis implementation correctly handles both enum instances and string values, ensuring the internal dictionary contains serializable string values instead of enum objects. This is important for preventing serialization issues when the data is stored in the database.
121-128: Good implementation of the new encumberedStatus propertyThe new
encumberedStatusproperty is well-implemented with appropriate nullable return type and straightforward getter/setter methods. This addition properly supports the adverse action feature being implemented.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (6)
28-34: LGTM: Well-structured test helper functionThe
_generate_test_bodyhelper function properly creates test request bodies with the required fields for encumbrance operations. This promotes code reuse and maintains consistency across tests.
38-39: Correct approach for patching datetime propertyThe patching of
cc_common.config._Config.current_standard_datetimewith a fixed datetime value is correctly implemented. Sincecurrent_standard_datetimeis a property (not a method), directly supplying a datetime value is the right approach for deterministic testing.
42-63: LGTM: Clean test setup with reusable helper methodThis helper method properly creates and returns both the test event and test privilege record, making it reusable across multiple test cases. The approach of setting up the test data once and reusing it improves test maintainability.
94-95: Intentional use of hardcoded license type abbreviationThe SK prefix embeds
'.../slp#ADVERSE_ACTION'using a hardcoded license type abbreviation. This is a deliberate design choice that makes tests fail loud if the default test license type changes, requiring developers to consciously update related tests.
181-195: Well-structured compact admin authorization testThis test correctly verifies that only state admins (not compact admins) can encumber privileges, which is an important security constraint. The test modifies only the scope claim while reusing the standard test setup, clearly isolating the variable being tested.
343-371: LGTM: Thorough verification of provider record updateThis test comprehensively verifies that the provider record is updated with the encumbered status when a license is encumbered. The test correctly asserts both the specific encumbrance status value and the complete provider record content.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (5)
15-141: Excellent implementation of base CCDataClassThe
CCDataClassimplementation provides a robust foundation for data model classes with several key benefits:
- Factory methods enforce proper instantiation patterns
- Validation is performed during serialization/deserialization
- Deepcopy ensures data safety during serialization
- Common properties are standardized
- Error handling is clear and helpful
This abstraction layer between data model and database schema makes it easier to work with typed data objects throughout the codebase while ensuring consistency in validation.
210-217: Good addition of AdverseActionAgainstEnumThis new enum clearly identifies the types of records that adverse actions can target (privileges or licenses). The enum values match the resource paths used in the API endpoints, maintaining consistency across the codebase.
238-245: Well-designed LicenseEncumberedStatusEnum with provider contextThe
LicenseEncumberedStatusEnumis well-designed with the additionalPROVIDER_ENCUMBEREDstatus that indicates a license belonging to a provider who has at least one encumbered license. This status propagation mechanism allows the system to track encumbrance relationships across related records.
247-252: Good parallel structure with PrivilegeEncumberedStatusEnumThe
PrivilegeEncumberedStatusEnummaintains a parallel structure toLicenseEncumberedStatusEnumwith its own contextual status (LICENSE_ENCUMBERED) that indicates when a privilege is associated with an encumbered license. This consistent approach makes the code more predictable and easier to understand.
259-269: Well-documented ClinicalPrivilegeActionCategory enumThe
ClinicalPrivilegeActionCategoryenum is properly documented with reference to the NPDB standards. The category names are descriptive and match the official terminology, facilitating data exchange with external systems.
We determined that we do not need to update the statuses of other licenses which were not encumbered, since currently whenever we check the status of the license we have access to the provider data as well, which we are now tracking the encumbrance status of.
|
@jlkravitz This is ready for your review. Thanks. |
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
Outdated
Show resolved
Hide resolved
...pact-connect/lambdas/python/common/cc_common/data_model/schema/home_jurisdiction/__init__.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/common_test/test_constants.py
Outdated
Show resolved
Hide resolved
...-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
Outdated
Show resolved
Hide resolved
...-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
Outdated
Show resolved
Hide resolved
...act-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Outdated
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
We have just verified that in the case of privilege encumbrance, the provider cannot purchase any more privileges, so we need to set the encumbrance status on the provider record in this case as well.
Fixed comments and introduced license util class to simplify logic for getting license types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
1039-1134: Comprehensive implementation of privilege encumbrance with atomic transactions.The
encumber_privilegemethod properly implements a transactional approach to ensure data consistency when encumbering a privilege. The method:
- Retrieves the privilege record
- Checks if it's already encumbered to avoid unnecessary updates
- Creates an update record for audit history
- Performs a transactional write that includes all related records
One minor issue: the method's docstring states it returns
None, but there's no explicit return statement. While Python functions implicitly returnNonewhen reaching the end without a return statement, an explicit return would improve clarity.def encumber_privilege(self, adverse_action: AdverseActionData) -> None: # ... implementation ... + return None
1135-1228: Comprehensive implementation of license encumbrance with proper error handling.The
encumber_licensemethod correctly implements the encumbrance workflow for licenses, similar to the privilege implementation. It includes proper error handling, logging, and atomic transactions to ensure data consistency.As with the privilege method, an explicit return statement would improve clarity:
def encumber_license(self, adverse_action: AdverseActionData) -> None: # ... implementation ... + return None
1181-1183: String literal used instead of enum for update type.In the privilege encumbrance method you're using
UpdateCategory.ENCUMBRANCEenum, but here you're using a string literal'encumbrance'. For consistency, use the enum in both places.- 'updateType': 'encumbrance', + 'updateType': UpdateCategory.ENCUMBRANCE,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/license_util.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(1 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_data_model/test_schema/test_base_record.py(1 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(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/common/common_test/test_constants.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/compact-connect/lambdas/python/common/cc_common/license_util.py (2)
webroot/src/plugins/EnvConfig/envConfig.plugin.ts (1)
config(41-60)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
license_type_abbreviations_for_compact(149-150)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (18)
backend/compact-connect/lambdas/python/common/cc_common/license_util.py (1)
1-35: Clean and well-designed utility for license type lookups.This new utility provides a solid abstraction for looking up license types by abbreviation within a compact. The code:
- Uses a dataclass for clean representation of license types
- Performs case-insensitive matching with proper error handling
- Gracefully handles missing compact configurations via try/except
- Returns None instead of raising exceptions when no match is found
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (12)
28-35: LGTM - Well-structured test body generation function.The test body generator provides the required fields for encumbrance operations with clear values.
38-39: Correct pattern for patching datetime property.Using the correct approach to patch the datetime property with a fixed value for deterministic testing.
42-65: LGTM - Comprehensive test setup with proper data generation.This helper method creates the necessary test data and API event for privilege encumbrance tests. It:
- Sets up a provider and privilege record in the database
- Creates an API Gateway event with proper pathParameters
- Uses the TestDataGenerator to consistently format license type abbreviations
- Sets the appropriate admin scope for authorization
66-79: LGTM - Verifies basic successful response.Good test coverage for the happy path, verifying that the handler returns a 200 status code and expected response body.
80-114: LGTM - Thorough verification of adverse action record creation.This test comprehensively verifies that:
- The adverse action record is created in the database
- The record contains the expected values
- The query to find the record uses the correct key pattern
Note: The hardcoded "slp" in the key pattern is intentional per team convention to make tests fail if default test data changes.
115-148: LGTM - Thorough verification of privilege update record creation.This test properly verifies the privilege update record is created with the expected values including the updateType of "encumbrance" and encumberedStatus change to "encumbered".
149-181: LGTM - Thorough verification of privilege record status update.This test effectively verifies that:
- The privilege record's encumberedStatus is updated to "encumbered"
- Other fields remain unchanged
- The dateOfUpdate is updated to the mocked datetime
182-211: LGTM - Verifies provider record encumbrance propagation.Good test to ensure that when a privilege is encumbered, the provider's overall encumberedStatus is also updated.
212-227: Well-designed authorization enforcement test.Proper test for authorization boundary - verifying that compact admins cannot encumber privileges (only state admins can).
229-231: Consistent test structure for license encumbrance.Following the same pattern as the privilege encumbrance tests promotes code consistency.
234-258: LGTM - Comprehensive test setup for license encumbrance.This helper follows the same pattern as the privilege helper, with appropriate adjustments for license-specific logic.
404-420: Clear documentation for license admin-only permission.The test comment clearly explains that only state admins (not compact admins) are allowed to encumber licenses.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
931-949: Well-designed generic helper for DynamoDB update transactions.This private helper method efficiently creates transaction items for updating encumbered status fields. It follows the DRY principle by providing a common implementation for different record types.
950-979: Good use of type-specific wrappers around the generic helper.These specialized methods add type safety while reusing the common encumbrance update logic for different entity types (privilege, license, provider). The consistent pattern makes the code more maintainable.
981-987: Simple utility for DynamoDB put operations.This helper method abstracts away the details of creating a DynamoDB put transaction item, which reduces code duplication across the encumbrance methods.
989-1038: Robust conditional provider encumbrance with appropriate error handling.This helper method correctly checks if a provider is already encumbered before adding an update transaction item, which prevents unnecessary database operations. The error handling and logging are thorough.
1083-1087: Use of enum for update category.Good use of the
UpdateCategory.ENCUMBRANCEenum value for the privilege update record type. This follows the reviewer's recommendation to use the enum instead of string literals.
State Admins need the ability to upload adverse actions against a practitioner to denote they are not able to practice with a specific license or privilege in a state.
As part of this change, we've started hitting pain points with using static json files for checking our expected responses from the API. This adds a TestDataGenerator class which can be used to create any number of variations in our tests to test specific use cases. We have also added data classes for abstracting the database layer from our application logic, which should greatly simplify how we are reading/writing data to our tables. This change is not a complete refactor, rather just a starting point for us to work with as we continue to develop the project.
Requirements List
Description List
Testing List
backend/compact-connect/tests/unit/test_api.pyCloses #672
Summary by CodeRabbit