Update cosm adverse action schemas to match business requirements#1326
Open
landonshumway-ia wants to merge 16 commits intocsg-org:mainfrom
Open
Update cosm adverse action schemas to match business requirements#1326landonshumway-ia wants to merge 16 commits intocsg-org:mainfrom
landonshumway-ia wants to merge 16 commits intocsg-org:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughNarrowed allowed encumbrance types to three values and standardized clinical privilege action categories to Changes
Sequence Diagram(s)(Skipped — changes do not present a multi-component sequential flow requiring diagramming under the rules.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py (1)
359-362: Remove enum constraint from response schema.Response schemas should remain unconstrained for flexibility. The enum validation belongs in the runtime layer (
ClinicalPrivilegeActionCategoryField), not in CDK schema definitions.Proposed change
'clinicalPrivilegeActionCategories': JsonSchema( type=JsonSchemaType.ARRAY, - items=JsonSchema( - type=JsonSchemaType.STRING, - enum=['Fraud', 'Consumer Harm', 'Other'], - ), + items=JsonSchema(type=JsonSchemaType.STRING), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py` around lines 359 - 362, The response schema currently constrains values by including an enum on the JsonSchema used for items (JsonSchema(type=JsonSchemaType.STRING, enum=[...])). Remove the enum parameter from that JsonSchema so the response schema only declares type=JsonSchemaType.STRING (or leaves items as an unconstrained JsonSchema) and let the runtime validator (ClinicalPrivilegeActionCategoryField) enforce the allowed values; update the JsonSchema instantiation that contains items=JsonSchema(...) accordingly.backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py (1)
643-643: Split request and response category schemas.
_clinical_privilege_action_categories_schemais reused by both_encumbrance_request_schemaand theadverseActionsresponse objects, so the new enum now constrains response models too. Keep the enum on the request side only, and leave response items as plain strings to avoid doc drift when response payloads evolve.♻️ Suggested split
- def _clinical_privilege_action_categories_schema(self) -> JsonSchema: + def _clinical_privilege_action_categories_response_schema(self) -> JsonSchema: return JsonSchema( type=JsonSchemaType.ARRAY, description='The categories of clinical privilege action', - items=JsonSchema( - type=JsonSchemaType.STRING, - enum=['Fraud', 'Consumer Harm', 'Other'], - ), + items=JsonSchema(type=JsonSchemaType.STRING), ) + + `@property` + def _clinical_privilege_action_categories_request_schema(self) -> JsonSchema: + return JsonSchema( + type=JsonSchemaType.ARRAY, + description='The categories of clinical privilege action', + items=JsonSchema(type=JsonSchemaType.STRING, enum=['Fraud', 'Consumer Harm', 'Other']), + )# request-only usage 'clinicalPrivilegeActionCategories': self._clinical_privilege_action_categories_request_schema # response-only usage 'clinicalPrivilegeActionCategories': self._clinical_privilege_action_categories_response_schemaBased on learnings: request schemas should include enum validation constraints while response schemas should remain unconstrained.
Also applies to: 771-771, 810-819, 828-836
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py` at line 643, The shared enum schema _clinical_privilege_action_categories_schema is being used for both request and response models; create two separate schemas—_clinical_privilege_action_categories_request_schema (with the enum/validation) and _clinical_privilege_action_categories_response_schema (plain string/no enum)—then update usages: replace references inside _encumbrance_request_schema (and any other request schemas) to use the request variant and replace response object fields such as adverseActions to use the response variant; ensure all occurrences (including the other spots referenced around the diff) are switched so request-side validation keeps enums while responses remain unconstrained.backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
45-51: Add one regression case for a retired category value.These helpers now only exercise the new happy-path payload. A single negative case with an old value like
Unsafe Practice or Substandard Carewould lock in the enum contraction introduced by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py` around lines 45 - 51, The current helper _generate_test_body only builds a happy-path payload using ClinicalPrivilegeActionCategory.FRAUD; add a regression test that submits a payload with the retired category string "Unsafe Practice or Substandard Care" to ensure the service rejects it (e.g., assert a validation error / 4xx response). Implement this by either extending _generate_test_body to accept a category override or creating a new test helper that copies TEST_ENCUMBRANCE_EFFECTIVE_DATE and replaces clinicalPrivilegeActionCategories with the literal "Unsafe Practice or Substandard Care", then call the existing handler/test harness and assert the expected failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/cosmetology-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json`:
- Around line 11-21: The snapshot schema for clinicalPrivilegeActionCategories
is missing the non-empty constraint; update the source schema in
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
to add minItems: 1 for the clinicalPrivilegeActionCategories array (the same
field that currently uses Length(min=1) at runtime), then regenerate the JSON
snapshot so the snapshot includes "minItems": 1 and matches the runtime
validator.
In `@backend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.py`:
- Around line 384-399: The current match predicate only compares
clinicalPrivilegeActionCategories and can match older records; update the
matching logic in the loop that builds matching_actions to also compare the
adverse_action's encumbranceEffectiveDate.effectiveStartDate against the
request_payload's encumbranceEffectiveDate.effectiveStartDate (or equivalent
key), i.e. compute request_effective =
request_payload.get('encumbranceEffectiveDate', {}).get('effectiveStartDate')
and adverse_effective = adverse_action.get('encumbranceEffectiveDate',
{}).get('effectiveStartDate') and require action_categories ==
expected_categories AND adverse_effective == request_effective when appending to
matching_actions so the test only passes for the newly created adverse action.
---
Nitpick comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 45-51: The current helper _generate_test_body only builds a
happy-path payload using ClinicalPrivilegeActionCategory.FRAUD; add a regression
test that submits a payload with the retired category string "Unsafe Practice or
Substandard Care" to ensure the service rejects it (e.g., assert a validation
error / 4xx response). Implement this by either extending _generate_test_body to
accept a category override or creating a new test helper that copies
TEST_ENCUMBRANCE_EFFECTIVE_DATE and replaces clinicalPrivilegeActionCategories
with the literal "Unsafe Practice or Substandard Care", then call the existing
handler/test harness and assert the expected failure behavior.
In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py`:
- Line 643: The shared enum schema _clinical_privilege_action_categories_schema
is being used for both request and response models; create two separate
schemas—_clinical_privilege_action_categories_request_schema (with the
enum/validation) and _clinical_privilege_action_categories_response_schema
(plain string/no enum)—then update usages: replace references inside
_encumbrance_request_schema (and any other request schemas) to use the request
variant and replace response object fields such as adverseActions to use the
response variant; ensure all occurrences (including the other spots referenced
around the diff) are switched so request-side validation keeps enums while
responses remain unconstrained.
In `@backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py`:
- Around line 359-362: The response schema currently constrains values by
including an enum on the JsonSchema used for items
(JsonSchema(type=JsonSchemaType.STRING, enum=[...])). Remove the enum parameter
from that JsonSchema so the response schema only declares
type=JsonSchemaType.STRING (or leaves items as an unconstrained JsonSchema) and
let the runtime validator (ClinicalPrivilegeActionCategoryField) enforce the
allowed values; update the JsonSchema instantiation that contains
items=JsonSchema(...) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c8627d0-b48c-4200-9755-6c1d1dc9f20e
📒 Files selected for processing (25)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/__init__.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/common.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/cosmetology-app/lambdas/python/common/common_test/test_constants.pybackend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.pybackend/cosmetology-app/lambdas/python/common/tests/resources/api/adverse-action-post.jsonbackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_investigation.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.pybackend/cosmetology-app/lambdas/python/search/handlers/manage_opensearch_indices.pybackend/cosmetology-app/lambdas/python/search/tests/function/test_manage_opensearch_indices.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.pybackend/cosmetology-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.pybackend/cosmetology-app/tests/smoke/investigation_smoke_tests.py
💤 Files with no reviewable changes (4)
- backend/cosmetology-app/lambdas/python/search/handlers/manage_opensearch_indices.py
- backend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.py
- backend/cosmetology-app/lambdas/python/search/tests/function/test_manage_opensearch_indices.py
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/fields.py
backend/cosmetology-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/cosm-encumbrance
branch
from
152119a to
e8b539a
Compare
landonshumway-ia
force-pushed
the
feat/cosm-encumbrance
branch
from
e8b539a to
1240fbd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.py`:
- Around line 588-592: The test sets
privilege_encumbrance_body['encumbranceType'] to an invalid enum value
'reprimand'; update the test to use one of the allowed EncumbranceType values
(e.g., 'suspension', 'revocation', or 'surrender of license') so the payload
validates against the EncumbranceType enum in common.py; locate and edit the
privilege_encumbrance_body definition in the encumbrance_smoke_tests.py test and
replace 'reprimand' with a permitted value (choose the one that fits the test
scenario).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a50e6180-a5d1-40b1-8862-c523fa646bf6
📒 Files selected for processing (17)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/common.pybackend/cosmetology-app/lambdas/python/common/common_test/test_constants.pybackend/cosmetology-app/lambdas/python/common/tests/resources/api/adverse-action-post.jsonbackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_investigation.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.pybackend/cosmetology-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_LICENSE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.pybackend/cosmetology-app/tests/smoke/investigation_smoke_tests.py
✅ Files skipped from review due to trivial changes (3)
- backend/cosmetology-app/lambdas/python/common/tests/resources/api/adverse-action-post.json
- backend/cosmetology-app/tests/smoke/investigation_smoke_tests.py
- backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_adverse_action.py
- backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_investigation.py
- backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py
- backend/cosmetology-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json
- backend/cosmetology-app/lambdas/python/common/common_test/test_constants.py
- backend/cosmetology-app/tests/resources/snapshots/PATCH_PRIVILEGE_INVESTIGATION_REQUEST_SCHEMA.json
- backend/cosmetology-app/tests/resources/snapshots/LICENSE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/cosmetology-app/tests/resources/snapshots/PRIVILEGE_ENCUMBRANCE_REQUEST_SCHEMA.json
- backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py
- backend/cosmetology-app/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/cosmetology-app/tests/smoke/smoke_common.py (1)
236-241: Consider simplifying the pk construction logic.The explicit
if compact is not None and provider_id is not Nonecheck is redundant since theelsebranch withoroperators handles all cases correctly—when both are provided, theorexpressions simply return the provided values.♻️ Simplified implementation
def get_all_provider_database_records(compact: str | None = None, provider_id: str | None = None): - if compact is not None and provider_id is not None: - pk = f'{compact}#PROVIDER#{provider_id}' - else: - resolved_compact = compact or 'cosm' - resolved_provider_id = provider_id or config.test_provider_id - pk = f'{resolved_compact}#PROVIDER#{resolved_provider_id}' + resolved_compact = compact or 'cosm' + resolved_provider_id = provider_id or config.test_provider_id + pk = f'{resolved_compact}#PROVIDER#{resolved_provider_id}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/smoke/smoke_common.py` around lines 236 - 241, The pk construction is over-complicated: remove the if/else and always compute resolved_compact = compact or 'cosm' and resolved_provider_id = provider_id or config.test_provider_id, then set pk = f'{resolved_compact}#PROVIDER#{resolved_provider_id}'; update the code that currently references the explicit if (using variables compact, provider_id, config.test_provider_id, and pk) to use this single simplified assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.py`:
- Around line 39-41: The membership check in _ensure_jurisdiction_live uses raw
compact and jurisdiction values and can fail for mixed/upper-case inputs;
normalize both inputs (compact and jurisdiction) to lowercase before calling
config.compact_configuration_client.is_jurisdiction_live_in_compact and then
raise CCInvalidRequestException('Jurisdiction is not live in this compact') only
if the normalized check returns False so casing differences won't erroneously
reject valid jurisdictions.
---
Nitpick comments:
In `@backend/cosmetology-app/tests/smoke/smoke_common.py`:
- Around line 236-241: The pk construction is over-complicated: remove the
if/else and always compute resolved_compact = compact or 'cosm' and
resolved_provider_id = provider_id or config.test_provider_id, then set pk =
f'{resolved_compact}#PROVIDER#{resolved_provider_id}'; update the code that
currently references the explicit if (using variables compact, provider_id,
config.test_provider_id, and pk) to use this single simplified assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2a83644-61f5-477b-b221-4c92e5d98837
📒 Files selected for processing (6)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/cosmetology-app/tests/smoke/config.pybackend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.pybackend/cosmetology-app/tests/smoke/smoke_common.pybackend/cosmetology-app/tests/smoke/smoke_tests_env_example.json
✅ Files skipped from review due to trivial changes (1)
- backend/cosmetology-app/tests/smoke/smoke_tests_env_example.json
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.py
backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py (1)
69-71: Minor data inconsistency: City and state mismatch.
homeAddressCityis set to'Omaha'(a city in Nebraska) whilehomeAddressStateis now'AZ'(Arizona). This inconsistency may be harmless for smoke tests since these are likely not validated together, but it could cause confusion when debugging test failures.Consider updating the city to an Arizona city (e.g.,
'Phoenix'or'Tucson') for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py` around lines 69 - 71, Update the test data in rollback_license_upload_smoke_tests.py so the address fields are consistent: change the 'homeAddressCity' value to an Arizona city (e.g., 'Phoenix' or 'Tucson') to match 'homeAddressState': 'AZ'; locate the object containing 'homeAddressState' and 'homeAddressCity' and replace the city string accordingly.backend/cosmetology-app/tests/smoke/smoke_common.py (1)
235-250: Default argument evaluated at module load time.The default
provider_id: str = config.test_provider_idis evaluated when the module is imported. Ifconfig.test_provider_id(which readsCC_TEST_PROVIDER_IDfrom environment) isn't set at import time, this will raise aKeyError.Currently this works because
load_smoke_test_env()is called at module level inconfig.py(line 105), but it creates a subtle coupling. Consider usingNoneas the default and resolving inside the function:♻️ Safer default argument pattern
-def get_all_provider_database_records(compact: str = 'cosm', provider_id: str = config.test_provider_id): +def get_all_provider_database_records(compact: str = 'cosm', provider_id: str | None = None): + if provider_id is None: + provider_id = config.test_provider_id items: list = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/smoke/smoke_common.py` around lines 235 - 250, The function get_all_provider_database_records uses a default argument provider_id: str = config.test_provider_id which is evaluated at import time; change the signature to accept provider_id: Optional[str] = None and inside the function resolve provider_id = provider_id or config.test_provider_id before using it (so the env lookup happens at call time), keeping the rest of the logic (pagination, Key('pk').eq(f'{compact}#PROVIDER#{provider_id}'), and config.provider_user_dynamodb_table.query) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py`:
- Around line 69-71: Update the test data in
rollback_license_upload_smoke_tests.py so the address fields are consistent:
change the 'homeAddressCity' value to an Arizona city (e.g., 'Phoenix' or
'Tucson') to match 'homeAddressState': 'AZ'; locate the object containing
'homeAddressState' and 'homeAddressCity' and replace the city string
accordingly.
In `@backend/cosmetology-app/tests/smoke/smoke_common.py`:
- Around line 235-250: The function get_all_provider_database_records uses a
default argument provider_id: str = config.test_provider_id which is evaluated
at import time; change the signature to accept provider_id: Optional[str] = None
and inside the function resolve provider_id = provider_id or
config.test_provider_id before using it (so the env lookup happens at call
time), keeping the rest of the logic (pagination,
Key('pk').eq(f'{compact}#PROVIDER#{provider_id}'), and
config.provider_user_dynamodb_table.query) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 496ede09-9028-42c7-8857-26310b93a855
📒 Files selected for processing (13)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.pybackend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.pybackend/cosmetology-app/stacks/api_lambda_stack/provider_management.pybackend/cosmetology-app/tests/smoke/compact_configuration_smoke_tests.pybackend/cosmetology-app/tests/smoke/config.pybackend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.pybackend/cosmetology-app/tests/smoke/investigation_smoke_tests.pybackend/cosmetology-app/tests/smoke/license_upload_smoke_tests.pybackend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.pybackend/cosmetology-app/tests/smoke/smoke_common.pybackend/cosmetology-app/tests/smoke/smoke_tests_env_example.jsonbackend/cosmetology-app/tests/smoke/ssn_read_throttling_smoke_tests.py
💤 Files with no reviewable changes (1)
- backend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.py
✅ Files skipped from review due to trivial changes (3)
- backend/cosmetology-app/tests/smoke/ssn_read_throttling_smoke_tests.py
- backend/cosmetology-app/tests/smoke/smoke_tests_env_example.json
- backend/cosmetology-app/tests/smoke/encumbrance_smoke_tests.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
- backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/encumbrance.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py (1)
79-84: Scope the upload key to the run too.
familyNameonly isolates the later provider query. The uploaded fixture still reuses the same business identifiers every run, so any leaked rollback data in the shared sandbox can be picked up again on Step 1 and turn this from a create/delete rollback case into an update/revert case. Please verify which incoming field the license ingest dedupes on and make that field run-scoped alongsidefamilyName.Also applies to: 620-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py` around lines 79 - 84, The uploaded fixture is not fully run-scoped: in addition to familyName (family_name) you must identify which incoming field the license ingest uses for deduplication (e.g., SSN or business identifier) and append the per-run unique token to that field as well so each test run creates distinct upload keys; update the test data where the fixture dict is constructed (the keys familyName and the deduped field such as ssn) to include the run-scoped suffix (use the existing family_name/run id), and apply the same change to the other occurrence of the fixture construction referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py`:
- Around line 746-749: The current call to verify_rollback_results(results,
expected_reverted, expected_skipped) is insufficient because
verify_rollback_results only logs a warning on mismatched reverted counts;
instead assert the reverted count directly so the test fails on mismatch:
compute the actual reverted count from the results object (e.g.,
results.get('num_reverted') or len(results.get('reverted_providers'))), and add
an explicit assertion like assert actual_reverted == expected_reverted (and keep
or assert skipped with expected_skipped) using the same symbols
NUM_LICENSES_TO_UPLOAD, expected_reverted, expected_skipped and results to
guarantee the test fails on partial rollback.
- Around line 646-651: The call to wait_for_all_providers_created(...) discards
the provider IDs it finds, so capture and store them immediately (e.g., assign
its return to the same variable used by the cleanup path such as
ALL_PROVIDER_IDS or a dedicated first_step_provider_ids) so the except cleanup
can delete Step 1 providers if later steps fail; update the call site where
wait_for_all_providers_created is invoked (with staff_headers,
len(uploaded_licenses), family_name=run_family_name) to assign its return value
and ensure the except block references that variable when performing cleanup.
---
Nitpick comments:
In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py`:
- Around line 79-84: The uploaded fixture is not fully run-scoped: in addition
to familyName (family_name) you must identify which incoming field the license
ingest uses for deduplication (e.g., SSN or business identifier) and append the
per-run unique token to that field as well so each test run creates distinct
upload keys; update the test data where the fixture dict is constructed (the
keys familyName and the deduped field such as ssn) to include the run-scoped
suffix (use the existing family_name/run id), and apply the same change to the
other occurrence of the fixture construction referenced in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee634404-4668-4a65-81eb-4fb245377955
📒 Files selected for processing (17)
backend/cosmetology-app/lambdas/python/cognito-backup/requirements-dev.txtbackend/cosmetology-app/lambdas/python/cognito-backup/requirements.txtbackend/cosmetology-app/lambdas/python/common/requirements-dev.txtbackend/cosmetology-app/lambdas/python/common/requirements.txtbackend/cosmetology-app/lambdas/python/compact-configuration/requirements-dev.txtbackend/cosmetology-app/lambdas/python/custom-resources/requirements-dev.txtbackend/cosmetology-app/lambdas/python/data-events/requirements-dev.txtbackend/cosmetology-app/lambdas/python/disaster-recovery/requirements-dev.txtbackend/cosmetology-app/lambdas/python/provider-data-v1/requirements-dev.txtbackend/cosmetology-app/lambdas/python/search/requirements-dev.txtbackend/cosmetology-app/lambdas/python/search/requirements.txtbackend/cosmetology-app/lambdas/python/staff-user-pre-token/requirements-dev.txtbackend/cosmetology-app/lambdas/python/staff-users/requirements-dev.txtbackend/cosmetology-app/requirements-dev.txtbackend/cosmetology-app/requirements.txtbackend/cosmetology-app/tests/smoke/query_provider_smoke_tests.pybackend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py
💤 Files with no reviewable changes (1)
- backend/cosmetology-app/tests/smoke/query_provider_smoke_tests.py
✅ Files skipped from review due to trivial changes (15)
- backend/cosmetology-app/lambdas/python/cognito-backup/requirements.txt
- backend/cosmetology-app/lambdas/python/search/requirements.txt
- backend/cosmetology-app/lambdas/python/common/requirements.txt
- backend/cosmetology-app/lambdas/python/provider-data-v1/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/search/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/custom-resources/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/data-events/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/disaster-recovery/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/cognito-backup/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/compact-configuration/requirements-dev.txt
- backend/cosmetology-app/requirements.txt
- backend/cosmetology-app/lambdas/python/staff-users/requirements-dev.txt
- backend/cosmetology-app/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/common/requirements-dev.txt
- backend/cosmetology-app/lambdas/python/staff-user-pre-token/requirements-dev.txt
Comment on lines
+646
to
+651
| wait_for_all_providers_created( | ||
| staff_headers, | ||
| len(uploaded_licenses), | ||
| max_wait_time=FIRST_UPLOAD_PROVIDER_INGEST_MAX_WAIT_SEC, | ||
| family_name=run_family_name, | ||
| ) |
Contributor
There was a problem hiding this comment.
Capture the Step 1 provider IDs for failure cleanup.
This first wait_for_all_providers_created() already finds the providers created by Step 1, but the result is discarded. If Step 2 or Step 3 fails before Line 686 assigns ALL_PROVIDER_IDS, the except block cannot clean up the first upload and leaves test data behind.
💡 Proposed fix
- wait_for_all_providers_created(
+ initial_provider_ids = wait_for_all_providers_created(
staff_headers,
len(uploaded_licenses),
max_wait_time=FIRST_UPLOAD_PROVIDER_INGEST_MAX_WAIT_SEC,
family_name=run_family_name,
)
+ ALL_PROVIDER_IDS = initial_provider_ids.copy()
...
- ALL_PROVIDER_IDS = provider_ids.copy()
+ ALL_PROVIDER_IDS = list(set(ALL_PROVIDER_IDS) | set(provider_ids))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py`
around lines 646 - 651, The call to wait_for_all_providers_created(...) discards
the provider IDs it finds, so capture and store them immediately (e.g., assign
its return to the same variable used by the cleanup path such as
ALL_PROVIDER_IDS or a dedicated first_step_provider_ids) so the except cleanup
can delete Step 1 providers if later steps fail; update the call site where
wait_for_all_providers_created is invoked (with staff_headers,
len(uploaded_licenses), family_name=run_family_name) to assign its return value
and ensure the except block references that variable when performing cleanup.
Comment on lines
+746
to
749
| # Expect all providers reverted except for the 1 skipped | ||
| expected_reverted = NUM_LICENSES_TO_UPLOAD - 1 | ||
| expected_skipped = 1 | ||
| verify_rollback_results(results, expected_reverted, expected_skipped) |
Contributor
There was a problem hiding this comment.
Make the reverted-provider count fail the test.
The call here looks strict, but verify_rollback_results() only warns when num_reverted != expected_provider_count (Lines 515-517). That means this new expected_reverted = NUM_LICENSES_TO_UPLOAD - 1 check still lets a partial rollback pass as long as skipped/failed counts happen to match.
💡 Proposed fix
- if num_reverted != expected_provider_count:
- logger.warning(f'Expected {expected_provider_count} reverted providers but found {num_reverted}')
+ if num_reverted != expected_provider_count:
+ raise SmokeTestFailureException(
+ f'Expected {expected_provider_count} reverted providers but found {num_reverted}'
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cosmetology-app/tests/smoke/rollback_license_upload_smoke_tests.py`
around lines 746 - 749, The current call to verify_rollback_results(results,
expected_reverted, expected_skipped) is insufficient because
verify_rollback_results only logs a warning on mismatched reverted counts;
instead assert the reverted count directly so the test fails on mismatch:
compute the actual reverted count from the results object (e.g.,
results.get('num_reverted') or len(results.get('reverted_providers'))), and add
an explicit assertion like assert actual_reverted == expected_reverted (and keep
or assert skipped with expected_skipped) using the same symbols
NUM_LICENSES_TO_UPLOAD, expected_reverted, expected_skipped and results to
guarantee the test fails on partial rollback.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py`:
- Around line 206-218: The resource creation for the smoke test is happening
before the try block, so failures in create_test_staff_user or
create_test_app_client bypass the failure logging and finally cleanup; move
create_test_staff_user and create_test_app_client (and any dependent variables
like client_id/client_secret) inside the existing try that already contains
get_client_auth_headers and upload_licenses_record, or expand the try to start
above those calls so that exceptions from
create_test_staff_user/create_test_app_client are caught and the finally cleanup
runs; ensure references to create_test_staff_user, create_test_app_client,
get_client_auth_headers, and upload_licenses_record are updated accordingly.
- Around line 181-188: The cleanup query is too broad and can delete other
tests' events; narrow it to the current test run by adding a run-specific filter
(e.g., upload_id or test_run_id) when querying data_events_table in
license_ingest_record_response and the similar block at 195-201: include the run
identifier in the KeyConditionExpression or add a FilterExpression with
ExpressionAttributeValues like ':run_id' and match the run-specific field (e.g.,
run_id, upload_id, request_id) present on the license.ingest items, and only
delete rows when the run identifier matches to avoid removing unrelated events;
also ensure _cleanup_test_generated_records() uses the same run-scoped query
before deleting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b824c26-b71f-4060-90e2-aad48d572c0d
📒 Files selected for processing (4)
backend/cosmetology-app/tests/smoke/README.mdbackend/cosmetology-app/tests/smoke/config.pybackend/cosmetology-app/tests/smoke/license_upload_smoke_tests.pybackend/cosmetology-app/tests/smoke/query_provider_smoke_tests.py
💤 Files with no reviewable changes (1)
- backend/cosmetology-app/tests/smoke/query_provider_smoke_tests.py
✅ Files skipped from review due to trivial changes (1)
- backend/cosmetology-app/tests/smoke/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/cosmetology-app/tests/smoke/config.py
Comment on lines
181
to
+188
| license_ingest_record_response = data_events_table.query( | ||
| KeyConditionExpression='pk = :pk AND sk BETWEEN :start_time AND :end_time', | ||
| ExpressionAttributeValues={ | ||
| ':pk': 'COMPACT#cosm#JURISDICTION#ne', | ||
| ':pk': f'COMPACT#{COMPACT}#JURISDICTION#{JURISDICTION}', | ||
| ':start_time': f'TYPE#license.ingest#TIME#{int(start_time.timestamp())}', | ||
| ':end_time': f'TYPE#license.ingest#TIME#{int(event_time.timestamp())}', | ||
| }, | ||
| ConsistentRead=True, |
Contributor
There was a problem hiding this comment.
Scope the data-events cleanup to this test run.
This query still returns every license.ingest event for the COMPACT#cosm#JURISDICTION#az partition in the 15-minute window, and _cleanup_test_generated_records() deletes every returned item. In a shared sandbox, one run can erase unrelated ingest events from other uploads/tests. Please narrow the match to a run-specific identifier before cleanup, or avoid deleting data-events rows when the match is ambiguous.
Also applies to: 195-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py` around
lines 181 - 188, The cleanup query is too broad and can delete other tests'
events; narrow it to the current test run by adding a run-specific filter (e.g.,
upload_id or test_run_id) when querying data_events_table in
license_ingest_record_response and the similar block at 195-201: include the run
identifier in the KeyConditionExpression or add a FilterExpression with
ExpressionAttributeValues like ':run_id' and match the run-specific field (e.g.,
run_id, upload_id, request_id) present on the license.ingest items, and only
delete rows when the run identifier matches to avoid removing unrelated events;
also ensure _cleanup_test_generated_records() uses the same run-scoped query
before deleting.
Comment on lines
+206
to
+218
| # Create staff user with permission to query providers (internal API) | ||
| test_user_sub = create_test_staff_user( | ||
| email=TEST_STAFF_USER_EMAIL, | ||
| compact=COMPACT, | ||
| jurisdiction=JURISDICTION, | ||
| permissions={'actions': {'admin'}, 'jurisdictions': {JURISDICTION: {'write', 'admin'}}}, | ||
| ) | ||
| client_credentials = create_test_app_client(TEST_APP_CLIENT_NAME, COMPACT, JURISDICTION) | ||
| client_id = client_credentials['client_id'] | ||
| client_secret = client_credentials['client_secret'] | ||
| try: | ||
| upload_licenses_record() | ||
| license_upload_headers = get_client_auth_headers(client_id, client_secret, COMPACT, JURISDICTION) | ||
| upload_licenses_record(license_upload_headers) |
Contributor
There was a problem hiding this comment.
Move resource creation inside the try.
create_test_staff_user() and create_test_app_client() can fail before control enters the try, which skips both the smoke-test failure logging and the finally cleanup. That leaves any earlier setup resources behind on the failure path.
🧹 Proposed fix
if __name__ == '__main__':
load_smoke_test_env()
- # Create staff user with permission to query providers (internal API)
- test_user_sub = create_test_staff_user(
- email=TEST_STAFF_USER_EMAIL,
- compact=COMPACT,
- jurisdiction=JURISDICTION,
- permissions={'actions': {'admin'}, 'jurisdictions': {JURISDICTION: {'write', 'admin'}}},
- )
- client_credentials = create_test_app_client(TEST_APP_CLIENT_NAME, COMPACT, JURISDICTION)
- client_id = client_credentials['client_id']
- client_secret = client_credentials['client_secret']
+ test_user_sub = None
+ client_id = None
try:
+ # Create staff user with permission to query providers (internal API)
+ test_user_sub = create_test_staff_user(
+ email=TEST_STAFF_USER_EMAIL,
+ compact=COMPACT,
+ jurisdiction=JURISDICTION,
+ permissions={'actions': {'admin'}, 'jurisdictions': {JURISDICTION: {'write', 'admin'}}},
+ )
+ client_credentials = create_test_app_client(TEST_APP_CLIENT_NAME, COMPACT, JURISDICTION)
+ client_id = client_credentials['client_id']
+ client_secret = client_credentials['client_secret']
license_upload_headers = get_client_auth_headers(client_id, client_secret, COMPACT, JURISDICTION)
upload_licenses_record(license_upload_headers)
logger.info('License record upload smoke test passed')
except SmokeTestFailureException as e:
logger.error(f'License record upload smoke test failed: {str(e)}')
finally:
- delete_test_app_client(client_id)
- # Clean up the test staff user
- delete_test_staff_user(TEST_STAFF_USER_EMAIL, user_sub=test_user_sub, compact=COMPACT)
+ if client_id:
+ delete_test_app_client(client_id)
+ if test_user_sub:
+ delete_test_staff_user(TEST_STAFF_USER_EMAIL, user_sub=test_user_sub, compact=COMPACT)Also applies to: 223-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py` around
lines 206 - 218, The resource creation for the smoke test is happening before
the try block, so failures in create_test_staff_user or create_test_app_client
bypass the failure logging and finally cleanup; move create_test_staff_user and
create_test_app_client (and any dependent variables like
client_id/client_secret) inside the existing try that already contains
get_client_auth_headers and upload_licenses_record, or expand the try to start
above those calls so that exceptions from
create_test_staff_user/create_test_app_client are caught and the finally cleanup
runs; ensure references to create_test_staff_user, create_test_app_client,
get_client_auth_headers, and upload_licenses_record are updated accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cosmetology will be using slightly different values for the encumbrance types and categories for encumbrances uploaded into the system. This updates those values along with any tests and api schemas that were referencing them.
Closes #1316
Summary by CodeRabbit