Refactor Cosmetology License schema to remove NPI / require license number#1291
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughRemoved privilege-related features and NPI, made licenseNumber required, added ui_domain_name_override and TLS 1.2 / domain validation, updated CDK/API wiring, schemas, handlers, tests, docs, generators, and Postman collections to reflect a license-centric model and removed privilege endpoints, history, export, notifications, and related tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
landonshumway-ia
force-pushed
the
feat/model-refactor
branch
from
c6d0394 to
75fd5cb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
341-349:⚠️ Potential issue | 🟡 MinorPatch
current_standard_datetimewith a concrete datetime, not a lambda.This property should be patched with a direct datetime value to match established test patterns and avoid mock inconsistencies.
Based on learnings: “cc_common.config._Config.current_standard_datetimeis a property … patch it by directly supplying a datetime value rather than wrapping it in a lambda.”✅ Suggested fix
patch_kwargs = {} if date_of_update is not None: # Use the provided fixed date - patch_kwargs['new_callable'] = lambda: date_of_update + patch_kwargs['new'] = date_of_update else: # Use random dates for variation - patch_kwargs['new_callable'] = lambda: ( - datetime.now(tz=UTC).replace(microsecond=0) - timedelta(days=randint(1, 365)) - ) + random_date = datetime.now(tz=UTC).replace(microsecond=0) - timedelta(days=randint(1, 365)) + patch_kwargs['new'] = random_datebackend/cosmetology-app/docs/it_staff_onboarding_instructions.md (1)
119-142:⚠️ Potential issue | 🟡 MinorPre-existing:
licenseTypevalue in the example may be incorrect.Line 126 uses
"licenseType":"cosmetology"but the valid values per the field descriptions and schema arecosmetologistandesthetician. This predates this PR but is worth noting since the onboarding doc is user-facing.backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json (1)
184-190:⚠️ Potential issue | 🟠 MajorCosmetology app's OAS spec has wrong compact enum values from CompactConnect.
The compact enum in this spec shows
["aslp", "octp", "coun"](CompactConnect's values), but the cosmetology app'scdk.jsondefines"compacts": ["cosm"]. The OAS spec should include"cosm"instead. Since this file is auto-generated, fix the enum values in the CDK source (likelybackend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.pyor the generation pipeline) so the spec updates correctly. This issue appears in multiple locations across the spec (at least 8 instances in search-internal OAS, 23 in internal OAS).
🧹 Nitpick comments (2)
backend/cosmetology-app/docs/api-specification/latest-oas30.json (2)
976-977: TLS 1.0 minimum security policy.
"x-amazon-apigateway-security-policy": "TLS_1_0"allows connections with TLS 1.0, which is considered deprecated and insecure. While this is auto-generated from CDK/API Gateway configuration and pre-dates this PR, consider updating the CDK source to enforce at least TLS 1.2 (TLS_1_2) for a stronger security posture.
137-185: The/v1/compacts/{compact}/jurisdictions/{jurisdiction}/providerspath contains only an OPTIONS method and appears orphaned in the auto-generated spec file. No corresponding CDK resource definition exists for this path (only/v1/compacts/{compact}/providersis defined undercompact_resourceinbackend/cosmetology-app/stacks/api_stack/v1_api/api.py). Since this API specification file is auto-generated, investigate whether this path was previously removed from CDK but left stale in the generated spec. If no longer needed, verify the CDK definitions are correct and the spec will regenerate without this path.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/cosmetology-app/docs/api-specification/latest-oas30.json (1)
671-671:⚠️ Potential issue | 🟠 MajorSet API Gateway custom domain to enforce TLS 1.2+.
The API Gateway is deployed with
TLS_1_0security policy, which permits insecure TLS versions. Inbackend/cosmetology-app/common_constructs/cc_api.py, addsecurity_policy=SecurityPolicy.TLS_1_2to theDomainNameOptionsconstructor (line 92) and importSecurityPolicyfromaws_cdk.aws_apigateway. This aligns with the OpenSearch domain configuration in the codebase, which already enforces TLS 1.2.
🧹 Nitpick comments (1)
backend/cosmetology-app/docs/api-specification/latest-oas30.json (1)
469-483: Static analysis: nested error arrays lackmaxItems.Checkov flags
CKV_OPENAPI_21on theerrorsschema — the inner array (lines 474–478) has nomaxItemsconstraint. Since this file is auto-generated, the fix should be applied in the CDK source (e.g., the API model definition). Consider adding amaxItemsto the error-message array in the source schema to satisfy this check.Based on learnings, this file is auto-generated by API Gateway and should not be modified inline; any schema changes need to be addressed at the source in the CDK/CloudFormation definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/cosmetology-app/common_constructs/cc_api.py`:
- Around line 114-121: The current logic unconditionally sets
disable_execute_api_endpoint based on environment_name which can disable the API
without a custom domain; change it to compute disable_execute_api_endpoint by
checking for a hosted zone instead (use stack.hosted_zone or equivalent), e.g.
set disable_execute_api_endpoint = bool(stack.hosted_zone) and/or combine with
environment guard if desired, then pass that variable into super().__init__
(references: environment_name, stack.hosted_zone, disable_execute_api_endpoint,
super().__init__) so prod/beta only disable the default endpoint when a custom
domain is actually configured.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
260-261:⚠️ Potential issue | 🟡 MinorRemove unused import
DataClientfrom line 260.
DataClientis imported but never referenced in the function. Remove this leftover import:- from cc_common.data_model.data_client import DataClient from handlers.ingest import ingest_license_message, preprocess_license_ingestbackend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json (1)
996-999:⚠️ Potential issue | 🟡 MinorThe generated spec is missing
minLength: 1forlicenseNumber. The CDK source (backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.pyline 259) correctly definesmin_length=1, max_length=100, but this constraint is not reflected in the generated spec. Regenerate the OpenAPI spec from the latest CDK source to resolve the mismatch.
🧹 Nitpick comments (9)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py (1)
94-101: Dead code: firstget_provider_user_recordscall is immediately overwritten.The fetch at lines 94–96 (via
client) is never used — line 99 overwritesprovider_user_recordswith a second fetch (viaself.config.data_client). Consider removing lines 94–96 to avoid confusion.Suggested fix
- provider_user_records: ProviderUserRecords = client.get_provider_user_records( - compact='cosm', provider_id=provider_id - ) - - # Get the provider and all update records straight from the table, to inspect them provider_user_records = self.config.data_client.get_provider_user_records( compact='cosm', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE )backend/common-cdk/common_constructs/stack.py (2)
139-149: Consider guarding against empty-string override.If
ui_domain_name_overrideis set to""in the environment context,is not Nonepasses and returns an empty string, which would produce an invalid origin (https://). A truthy check or explicit validation would be safer.Suggested fix
- override = self.environment_context.get('ui_domain_name_override') - if override is not None: - return override + override = self.environment_context.get('ui_domain_name_override') + if override: + return override
161-165: Error message doesn't mentionui_domain_name_overrideas an option.The ValueError guidance suggests
domain_nameorallow_local_ui, butui_domain_name_overridewould also resolve this. Consider mentioning it for completeness, especially since this is a new feature being introduced in this PR.Suggested tweak
if not allowed_origins: raise ValueError( 'This app requires at least one allowed origin for its API CORS configuration. Either provide ' - "'domain_name' or set 'allow_local_ui' to true in this environment's context." + "'domain_name', provide 'ui_domain_name_override', or set 'allow_local_ui' to true in this " + "environment's context." )backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
631-649: Unusedprivilege_updatesvariable.
privilege_updates(line 633) is assigned but never referenced after the removal of privilege history enrichment logic. This is dead code left over from the refactor.♻️ Suggested fix
# Build privileges dict with investigations and adverseActions for privilege_record in self._privilege_records: privilege_dict = privilege_record.to_dict() - privilege_updates = self.get_update_records_for_privilege( - privilege_record.jurisdiction, privilege_record.licenseType - ) privilege_dict['adverseActions'] = [backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (1)
170-177: Dead code: privilege status assignment is immediately overwritten.Line 171 sets
expected_provider['privileges'][0]['status'] = 'inactive', but line 176 calls_set_provider_data_to_empty_valueswhich resetsprivilegesto[], discarding that assignment. This is harmless but misleading — consider removing line 171 to avoid confusion.♻️ Suggested cleanup
expected_provider['compactEligibility'] = 'ineligible' expected_provider['licenses'][0]['compactEligibility'] = 'ineligible' - # ensure the privilege record is also set to inactive - expected_provider['privileges'][0]['status'] = 'inactive' provider_data = self._get_provider_via_api(provider_id)backend/cosmetology-app/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (3)
39-39: Stale comment:'privilegeUpdate'no longer applicable.The docstring for
record_typestill mentions'privilegeUpdate'as a possible value, but privilege updates have been removed from this flow.📝 Update comment
- record_type: str # 'licenseUpdate' or 'privilegeUpdate' + record_type: str # 'licenseUpdate'
599-602: Stale comment still references privilege records.Line 599 still mentions "license/privilege/provider first tier" but privileges are no longer part of this flow.
📝 Update comment
- # Split transaction lists into first tier/second tier lists (license/privilege/provider first tier, updates second) - # then merge the two lists into a single list of transaction items + # Split transaction lists into primary records (license and provider) and update records (license updates), + # then merge the two lists into a single list of transaction items
666-666:reverted_licenses_dictis dead code — never read after population.
reverted_licenses_dictis initialized on line 666 and appended to on line 745, but is never consumed. The correct variable,reverted_licenses, is the one used in the return statements (lines 836, 841). TheLicenseRecordSchemainstantiation on line 744 also exists solely to feed this dead list. This appears to be a remnant of incomplete refactoring when privilege-processing logic was removed.♻️ Remove dead code
- reverted_licenses_dict = [] for license_record in license_records:- # Track for provider record regeneration - license_schema = LicenseRecordSchema() - reverted_licenses_dict.append(license_schema.load(serialized_reverted_license)) - reverted_licenses.append( RevertedLicense( jurisdiction=license_record.jurisdiction,Also applies to: 744–745
backend/cosmetology-app/tests/resources/snapshots/PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json (1)
108-109: Nit: jurisdiction filter description still references "privilege".The description says "Filter for providers with privilege/license in a jurisdiction". Since privilege-related endpoints are being removed from Cosmetology, consider updating to just "Filter for providers with a license in a jurisdiction" for clarity.
📝 Suggested wording update
- "description": "Filter for providers with privilege/license in a jurisdiction", + "description": "Filter for providers with a license in a jurisdiction",
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py`:
- Line 99: Update the inconsistent description in
public_query_providers_response_model so it matches the revised wording used
elsewhere: remove the "privilege/" prefix and change the field description from
'Filter for providers with privilege/license in a jurisdiction' to 'Filter for
providers with license in a jurisdiction' (or the exact phrasing used at the
other location). Locate the field inside public_query_providers_response_model
and replace its description string accordingly so both occurrences (around the
earlier change and lines ~1398-1401) are identical.
🧹 Nitpick comments (3)
backend/cosmetology-app/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
808-811: Stale comment still references privileges.Line 809–811 mentions "privilege or other non-upload records," but privilege handling has been removed from this file. Consider updating the comment to reflect the current scope.
✏️ Suggested comment update
- # We never expect this to happen, since license records should not have been removed if there were any - # privilege or other non-upload records found for the provider. If we hit this case, we will end the - # entire process to ensure we are not putting the system into a worse state. + # We never expect this to happen, since license records should not have been removed if there were + # other non-upload records found for the provider. If we hit this case, we will end the + # entire process to ensure we are not putting the system into a worse state.backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/__init__.py (1)
331-339: Consider usingnewinstead ofnew_callablefor patchingcurrent_standard_datetime.Since
current_standard_datetimeis a property, the idiomatic way to patch it in this codebase is to pass the datetime value directly via thenewkeyword rather than wrapping it in a lambda vianew_callable.new_callableis intended for supplying a class/factory thatpatchinstantiates, so using it to return a plain value is an unnecessary indirection.♻️ Suggested simplification
patch_kwargs = {} if date_of_update is not None: - patch_kwargs['new_callable'] = lambda: date_of_update + patch_kwargs['new'] = date_of_update else: - patch_kwargs['new_callable'] = lambda: ( + patch_kwargs['new'] = ( datetime.now(tz=UTC).replace(microsecond=0) - timedelta(days=randint(1, 365)) )Based on learnings: "The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda:
patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(...))."backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
146-154: Minor: redundant explicit keys increate_newdict.
providerIdandcompacton lines 149–150 are immediately overridden by the**license_recordspread on line 152, since the source dict contains those same keys. They're harmless (makes intent clear), but worth noting for future cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@backend/cosmetology-app/docs/api-specification/latest-oas30.json`:
- Line 345: The OAS snapshot in latest-oas30.json contains
x-amazon-apigateway-security-policy: "TLS_1_0" which is stale compared to the
CDK construct that sets security_policy=SecurityPolicy.TLS_1_2 in cc_api.py;
regenerate the OpenAPI export (or re-run the deployment/export step that
produces latest-oas30.json) so the OAS reflects the CDK setting, or if
regeneration is not possible add a short note in the doc snapshot indicating
it’s a stale export referencing SecurityPolicy.TLS_1_2 in cc_api.py and ensure
CI regenerates the spec on next deployment.
In
`@backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json`:
- Around line 67-101: The OpenAPI schema "SandboSearcFlaGc2hNzmIy" defines array
properties "search_after" and "sort" without maxItems; update the CDK model that
generates this schema to set an appropriate maxItems for both (for example,
match the expected number of sort fields) so that the generated JSON includes
"maxItems" for the "search_after" and "sort" properties and satisfies
CKV_OPENAPI_21; locate where the request model for the search endpoint is
defined (the structure that produces "SandboSearcFlaGc2hNzmIy") and add the
maxItems constraint to the array definitions there.
In
`@backend/cosmetology-app/docs/search-internal/postman/postman-collection.json`:
- Around line 586-590: The Postman collection uses a developer-specific file
path in the request body (the JSON entry with "key": "file" and "src":
"AmVhGkArk/octp-nc-mock-data.csv"); replace that src value with a generic
placeholder or a path to a committed mock CSV in the repo (for example
"mock-data/cosmetology-mock.csv" or simply "{{mock_csv_path}}") so other team
members can import the collection without needing the original developer's local
file.
- Around line 30-34: The Postman test script uses console.log to print sensitive
tokens (see pm.test('Access token returned', access_token usage and
pm.environment.set("accessToken")), which can expose credentials when the
collection is shared or run in CI; remove the console.log calls that output
access_token/refresh_token/id_token (and any similar console.log lines around
lines 34, 106, 113) or replace them with non-sensitive stub/masked output,
ensuring pm.environment.set still stores the token but no raw token value is
written to logs.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@backend/cosmetology-app/docs/api-specification/latest-oas30.json`: - Line 345: The OAS snapshot in latest-oas30.json contains x-amazon-apigateway-security-policy: "TLS_1_0" which is stale compared to the CDK construct that sets security_policy=SecurityPolicy.TLS_1_2 in cc_api.py; regenerate the OpenAPI export (or re-run the deployment/export step that produces latest-oas30.json) so the OAS reflects the CDK setting, or if regeneration is not possible add a short note in the doc snapshot indicating it’s a stale export referencing SecurityPolicy.TLS_1_2 in cc_api.py and ensure CI regenerates the spec on next deployment. In `@backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json`: - Around line 67-101: The OpenAPI schema "SandboSearcFlaGc2hNzmIy" defines array properties "search_after" and "sort" without maxItems; update the CDK model that generates this schema to set an appropriate maxItems for both (for example, match the expected number of sort fields) so that the generated JSON includes "maxItems" for the "search_after" and "sort" properties and satisfies CKV_OPENAPI_21; locate where the request model for the search endpoint is defined (the structure that produces "SandboSearcFlaGc2hNzmIy") and add the maxItems constraint to the array definitions there. In `@backend/cosmetology-app/docs/search-internal/postman/postman-collection.json`: - Around line 30-34: The Postman test script uses console.log to print sensitive tokens (see pm.test('Access token returned', access_token usage and pm.environment.set("accessToken")), which can expose credentials when the collection is shared or run in CI; remove the console.log calls that output access_token/refresh_token/id_token (and any similar console.log lines around lines 34, 106, 113) or replace them with non-sensitive stub/masked output, ensuring pm.environment.set still stores the token but no raw token value is written to logs.backend/cosmetology-app/docs/search-internal/postman/postman-collection.json (1)
30-34: Token values logged to console in Postman test scripts.Lines 34, 106, 113, etc. log access tokens, identity tokens, and refresh tokens via
console.log. This is fine for local debugging, but be aware that if this collection is shared or used in CI, token values will appear in logs.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@backend/cosmetology-app/docs/search-internal/postman/postman-collection.json` around lines 30 - 34, The Postman test script uses console.log to print sensitive tokens (see pm.test('Access token returned', access_token usage and pm.environment.set("accessToken")), which can expose credentials when the collection is shared or run in CI; remove the console.log calls that output access_token/refresh_token/id_token (and any similar console.log lines around lines 34, 106, 113) or replace them with non-sensitive stub/masked output, ensuring pm.environment.set still stores the token but no raw token value is written to logs.backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json (1)
67-101: Request schemasearch_afterandsortarrays lackmaxItems.The
search_after(line 73) andsort(line 92) arrays don't definemaxItems, which Checkov flags (CKV_OPENAPI_21). Since this file is auto-generated, this would need to be addressed in the CDK source model if you want to enforce a bound on these request arrays. In practice, OpenSearchsearch_aftervalues are typically small (matching the sort fields count), so the risk is low.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json` around lines 67 - 101, The OpenAPI schema "SandboSearcFlaGc2hNzmIy" defines array properties "search_after" and "sort" without maxItems; update the CDK model that generates this schema to set an appropriate maxItems for both (for example, match the expected number of sort fields) so that the generated JSON includes "maxItems" for the "search_after" and "sort" properties and satisfies CKV_OPENAPI_21; locate where the request model for the search endpoint is defined (the structure that produces "SandboSearcFlaGc2hNzmIy") and add the maxItems constraint to the array definitions there.backend/cosmetology-app/docs/api-specification/latest-oas30.json (1)
345-345: TLS policy in OAS spec appears outdated — CDK is correctly configured for TLS_1_2The OAS file shows
x-amazon-apigateway-security-policy: "TLS_1_0"at line 345, but this appears to be a documentation accuracy issue. The CDK source (backend/cosmetology-app/common_constructs/cc_api.pyline 98) explicitly configuressecurity_policy=SecurityPolicy.TLS_1_2, which is also verified by tests. Since OAS specs are auto-generated by API Gateway, the mismatch likely indicates the file is stale and needs regeneration, or reflects a known limitation in how API Gateway exports the OAS. The actual deployed API Gateway should enforce TLS 1.2 per the CDK configuration. Confirm whether this OAS snapshot is current or needs to be regenerated during the next deployment.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@backend/cosmetology-app/docs/api-specification/latest-oas30.json` at line 345, The OAS snapshot in latest-oas30.json contains x-amazon-apigateway-security-policy: "TLS_1_0" which is stale compared to the CDK construct that sets security_policy=SecurityPolicy.TLS_1_2 in cc_api.py; regenerate the OpenAPI export (or re-run the deployment/export step that produces latest-oas30.json) so the OAS reflects the CDK setting, or if regeneration is not possible add a short note in the doc snapshot indicating it’s a stale export referencing SecurityPolicy.TLS_1_2 in cc_api.py and ensure CI regenerates the spec on next deployment.
backend/cosmetology-app/docs/search-internal/postman/postman-collection.json
Outdated
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review. The linters are failing due to dependency updates which will be fixed once #1277 is merged. |
The multi-state license model does not generate privileges that exist in the database, so these endpoints are not needed.
We will retain the code to support such request signing if needed, but we will not reference it in our docs as we don't plan to support programmatic reading from the API. The docs may be added back in later if we decide to support GET methods through the state API.
The Cosmetology API will be called by the same UI as the original compact connect app used by JCC. As such, we need to set the explicit ui domain in the env context so we set the correct callback urls, CORS allowed origins, and email template links
- Removed privilege deactivation logic/endpoints/email template - Remove Event schemas/email templates related to privilege issuance/renewal - Remove template for transaction settlement error message
This includes the removal of the 'activeSince' field, since this can no longer be determined as we do not have a full history of license records issued outside the CompactConnect system
Cosmetology does not have issued privilege records like JCC, so we do not check for them when rolling back bad license uploads
landonshumway-ia
force-pushed
the
feat/model-refactor
branch
from
dcaea4c to
9c23505
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py (1)
172-185:⚠️ Potential issue | 🟡 MinorRemove the unused
_load_provider_dataoverride or drop the privilege.json loading.The
_load_provider_datamethod (lines 172–186) is not called by any test in the TestClient class. All test methods use_generate_providersinstead. This is dead code that should be removed entirely.Additionally, lines 178–180 load and insert a
privilege.jsonrecord, which conflicts with the PR's stated goal of removing privilege support from the Cosmetology compact. Either remove the entire method or, at minimum, remove the privilege.json loading block.🤖 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_data_model/test_client.py` around lines 172 - 185, The method _load_provider_data in the TestClient class is dead code and also inserts a privilege.json record which conflicts with the PR's removal of privilege support; delete the entire _load_provider_data method from the test file (backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py) so tests rely only on _generate_providers, or if you prefer a minimal change remove only the block that opens and puts privilege.json (the lines that reference privilege.json and put_item for that record) ensuring no other code calls _load_provider_data.backend/cosmetology-app/tests/resources/snapshots/PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json (1)
108-123:⚠️ Potential issue | 🟡 MinorStale description in embedded
query.jurisdictionfield.Line 109 still reads
"Filter for providers with privilege/license in a jurisdiction", but the siblingQUERY_PROVIDERS_REQUEST_SCHEMA.json(line 14) was updated in this PR to"Filter for providers with license in a jurisdiction". This description should be updated for consistency.✏️ Proposed fix
- "description": "Filter for providers with privilege/license in a jurisdiction", + "description": "Filter for providers with license in a jurisdiction",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/resources/snapshots/PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json` around lines 108 - 123, Update the stale description for the embedded query.jurisdiction field in PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json to match the request schema: change the description text from "Filter for providers with privilege/license in a jurisdiction" to "Filter for providers with license in a jurisdiction" so the query.jurisdiction enum/description is consistent with QUERY_PROVIDERS_REQUEST_SCHEMA.json.
🧹 Nitpick comments (7)
backend/cosmetology-app/tests/common_constructs/test_data_migration.py (1)
26-27: Minor inconsistency betweenenvironment_nameand theenvironmenttag.
environment_name='sandbox'(line 26) whileStandardTags(environment='test')(line 27). SinceStandardTags.environmentis purely a metadata label applied to CloudFormation resource tags and carries no functional gate, this has no runtime impact — but aligning both to'sandbox'would be more consistent and self-documenting.🔧 Optional alignment
- standard_tags=StandardTags(project='compact-connect', service='compact-connect', environment='test'), + standard_tags=StandardTags(project='compact-connect', service='compact-connect', environment='sandbox'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/tests/common_constructs/test_data_migration.py` around lines 26 - 27, The test sets environment_name='sandbox' but the StandardTags instance uses environment='test'; update the tags to match by changing the StandardTags(...) call (the environment argument) from 'test' to 'sandbox' so environment_name and StandardTags.environment are consistent (look for the StandardTags(...) instantiation in test_data_migration.py).backend/cosmetology-app/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py (1)
808-811: Nit: stale "privilege" reference in comment.Line 809 still references "privilege or other non-upload records," but Cosmetology no longer has privileges. The defensive code logic itself is still valid (guarding against unexpected non-license records), but the comment is now misleading.
Suggested wording
- # We never expect this to happen, since license records should not have been removed if there were any - # privilege or other non-upload records found for the provider. If we hit this case, we will end the + # We never expect this to happen, since license records should not have been removed if there were any + # other non-upload records found for the provider. If we hit this case, we will end the🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py` around lines 808 - 811, Update the misleading comment inside the if block that checks len(provider_records_after_rollback.provider_records) > 1 in rollback_license_upload.py: remove the stale word "privilege" and reword the message to state we never expect multiple provider_records and that this is a defensive guard against unexpected non-license/non-upload records (so the comment accurately describes the check on provider_records_after_rollback.provider_records).backend/cosmetology-app/docs/README.md (1)
52-52: Minor:OT12345example value is associated with Occupational Therapy, not Cosmetology.The
OTprefix is the well-known abbreviation for the Occupational Therapy compact. Using it here in a cosmetology-specific guide may confuse readers. The FAQ example CSV on line 97 already usesCOS12345, which is a clearer convention.📝 Suggested fix
-| licenseNumber* | License number | String (max 100 chars) | OT12345 | +| licenseNumber* | License number | String (max 100 chars) | COS12345 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/docs/README.md` at line 52, The example value for the licenseNumber field uses "OT12345" which is for Occupational Therapy and may confuse readers; update the example in the table row for "licenseNumber*" to a cosmetology-appropriate value (e.g., "COS12345") so it matches the FAQ CSV example and domain context; ensure the table cell containing the example is replaced accordingly for the licenseNumber entry.backend/cosmetology-app/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py (1)
620-623: Consider annotating the identical SHAs in the pagination testThe SHA update on line 622 is correct — removing NPI changes the serialized record and thus the content-based sort-key hash. The single-provider assertion already carries the helpful note on line 621.
In the pagination test (lines 927 and 940), both
mock_first_provider_idandmock_second_provider_idassert the same sort-key string. This is correct whenproviderIdlives in the DynamoDB partition key (not the sort key), so two providers with identical update content produce the same SK — but that's non-obvious to a future reader. Adding a brief comment (mirroring line 621) would prevent confusion:📝 Suggested annotation
'updatesDeleted': [ + # Same SK for both providers: provider_id is in PK, not SK, + # so identical update content yields identical sort keys. + # NOTE: if test update data is modified, this SHA must be updated. 'cosm#UPDATE#3#license/oh/cos/2025-10-23T07:15:00+00:00/ecd7b0d5fbe7c32dff89c9864ebb8daf' ],Also applies to: 924-942
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py` around lines 620 - 623, The pagination test currently asserts the same sort-key SHA for mock_first_provider_id and mock_second_provider_id which is correct but non-obvious; update the pagination test near the assertions for mock_first_provider_id and mock_second_provider_id to add a brief comment (similar to the existing note next to the single-provider assertion) explaining that providerId is part of the DynamoDB partition key, not the sort key, so identical update content produces the same SK/SHA — reference the specific sort-key string/sha being asserted to make the rationale clear to future readers.backend/common-cdk/common_constructs/stack.py (1)
151-165:ValueErrormessage omitsui_domain_name_overrideas a resolution path.The error message at lines 162–164 guides the developer to provide
domain_nameor setallow_local_ui, but a third valid option now exists. Any developer deploying a cross-domain stack (e.g. cosmetology backend + shared UI domain) who hits this error will be misled.📝 Suggested fix
- if not allowed_origins: - raise ValueError( - 'This app requires at least one allowed origin for its API CORS configuration. Either provide ' - "'domain_name' or set 'allow_local_ui' to true in this environment's context." - ) + if not allowed_origins: + raise ValueError( + 'This app requires at least one allowed origin for its API CORS configuration. Either provide ' + "'domain_name', set 'ui_domain_name_override', or set 'allow_local_ui' to true in this " + "environment's context." + )Additionally, consider adding a truthy check to guard against an accidental empty-string
ui_domain_name_override. An empty string passesoverride is not None, causingallowed_originsto append"https://"(an invalid CORS origin) without triggering theValueErrorguard:📝 Optional guard
- override = self.environment_context.get('ui_domain_name_override') - if override is not None: - return override + override = self.environment_context.get('ui_domain_name_override') + if override: # truthy check also rejects empty string + return override🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/common-cdk/common_constructs/stack.py` around lines 151 - 165, The ValueError in the allowed_origins property incorrectly omits the ui_domain_name_override resolution and also allows an empty-string override to produce an invalid "https://" origin; update the error text to mention providing ui_domain_name_override (or its config key) as a valid option, and change the ui_domain_name check (the property using self.ui_domain_name) to require a truthy value (e.g., if self.ui_domain_name) so empty strings are rejected and will trigger the ValueError; ensure references to environment_context keys (allow_local_ui, local_ui_port) remain unchanged.backend/compact-connect/common_constructs/cc_api.py (1)
114-121: Consider guardingdisable_execute_api_endpointagainst missing custom domain.If
environment_nameis'test','beta', or'prod'butstack.hosted_zoneisNone(no custom domain configured), the default execute-api endpoint will be disabled with no alternative, making the API unreachable. While pipeline environments should always have a hosted zone, coupling these conditions would make the code more defensive:Suggested safeguard
- # Disable the default execute-api endpoint for all pipeline environments so traffic must use the custom domain. - disable_execute_api_endpoint = environment_name in ('test', 'beta', 'prod') + # Disable the default execute-api endpoint for pipeline environments that have a custom domain configured, + # so traffic must use the custom domain. + disable_execute_api_endpoint = environment_name in ('test', 'beta', 'prod') and stack.hosted_zone is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/common_constructs/cc_api.py` around lines 114 - 121, The current logic sets disable_execute_api_endpoint based only on environment_name which can disable the API when no custom domain exists; update the condition to only disable the default execute-api endpoint when environment_name is in ('test','beta','prod') AND the stack has a hosted zone (check stack.hosted_zone is not None) before passing disable_execute_api_endpoint into super().__init__; refer to the variables environment_name, stack.hosted_zone, disable_execute_api_endpoint and the super().__init__ call to locate and change the logic.backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py (1)
21-26: Stale docstring — privilege search endpoint no longer exists.The
_common_search_request_schemadocstring states it is "used by both provider and privilege search endpoints", but the privilege search models (search_privileges_request_model/search_privileges_response_model) were removed in this PR.✏️ Proposed fix
- Return the common search request schema used by both provider and privilege search endpoints. + Return the common search request schema used by the provider search endpoint.🤖 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 21 - 26, Update the stale docstring on _common_search_request_schema to remove the reference to the removed privilege search endpoints (search_privileges_request_model / search_privileges_response_model) and instead describe the schema as used by the provider search endpoint or as a shared/common search request schema for pagination (e.g., "used by provider search endpoint" or "used by search endpoints requiring search_after pagination"); keep the link to OpenSearch DSL pagination and any other relevant details intact.
🤖 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/docs/api-specification/latest-oas30.json`:
- Around line 136-158: The Checkov finding points out the inner array schema
under SandboStateDr9HAH5blAWv -> properties -> errors -> additionalProperties ->
additionalProperties (the array of error messages) lacks a maxItems constraint;
update the CDK API model that generates this JSON (not this generated file) to
add a suitable "maxItems" value on that array schema (or add a
suppression/annotation in the CDK model if you intentionally want it unbounded)
so the generated OpenAPI includes a maxItems for
errors.additionalProperties.additionalProperties.items and silences
CKV_OPENAPI_21.
In `@backend/cosmetology-app/docs/postman/postman-collection.json`:
- Around line 804-808: The Postman collection contains a hardcoded local file
path in the request body object where the "src" key is set to
"AmVhGkArk/octp-nc-mock-data.csv"; replace that value with a neutral placeholder
such as an empty string or "/path/to/mock-data.csv" so other developers won't
get a file-not-found error when running the "Upload Document" step, and leave a
short comment or description instructing users to supply their own file path
before executing the request.
- Around line 328-344: Add explicit non-empty assertions for the tokens in the
refresh-token response: after retrieving pm.response.json().access_token and
.id_token (the variables access_token and id_token used in the existing pm.test
blocks), assert that each is not empty (use
pm.expect(access_token).to.not.be.empty and pm.expect(id_token).to.not.be.empty)
before calling pm.environment.set("accessToken", ...) and
pm.environment.set("idToken", ...), preserving the existing pm.test names
("Access token returned" and "Identity token returned") and console.log calls.
- Around line 57-88: The token request currently places grant_type, client_id,
client_secret and scope in the request URL query (see the url.query array and
raw field), which is disallowed; move these four items into the POST body as
form-encoded parameters (body.urlencoded) instead of url.query, or alternatively
use HTTP Basic auth for client_id/client_secret on the same POST to the
oauth2/token endpoint—update the request definition that contains the
"method":"POST" and "url":{...} block to remove those query entries and add them
under body.urlencoded with keys grant_type, client_id, client_secret, and scope.
---
Outside diff comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py`:
- Around line 172-185: The method _load_provider_data in the TestClient class is
dead code and also inserts a privilege.json record which conflicts with the PR's
removal of privilege support; delete the entire _load_provider_data method from
the test file
(backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py)
so tests rely only on _generate_providers, or if you prefer a minimal change
remove only the block that opens and puts privilege.json (the lines that
reference privilege.json and put_item for that record) ensuring no other code
calls _load_provider_data.
In
`@backend/cosmetology-app/tests/resources/snapshots/PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json`:
- Around line 108-123: Update the stale description for the embedded
query.jurisdiction field in PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json to match
the request schema: change the description text from "Filter for providers with
privilege/license in a jurisdiction" to "Filter for providers with license in a
jurisdiction" so the query.jurisdiction enum/description is consistent with
QUERY_PROVIDERS_REQUEST_SCHEMA.json.
---
Duplicate comments:
In `@backend/cosmetology-app/common_constructs/cc_api.py`:
- Around line 114-121: The current logic sets disable_execute_api_endpoint based
only on environment_name (in __init__ of the CC API construct) which breaks APIs
if a custom domain isn't configured; update the guard so disabling only happens
when both the environment_name is one of ('test','beta','prod') AND a custom
domain is actually available (check for domain_name in the environment/context
or that hosted_zone is not None). Concretely, in the CC API constructor: derive
domain_name/hosted_zone from the environment/context as you already do, then set
disable_execute_api_endpoint = environment_name in ('test','beta','prod') and
(domain_name is not None or hosted_zone is not None); pass that value to
super().__init__ instead of the unconditional environment-name-only check.
In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py`:
- Around line 1398-1401: The description for the 'jurisdiction' JsonSchema
inside public_query_providers_response_model is inconsistent; update its
description (the value passed to description in the 'jurisdiction' JsonSchema)
to read "Filter for providers with license in a jurisdiction" so it matches the
updated wording elsewhere (e.g., the earlier declaration at line ~99) and
removes the word "privilege".
---
Nitpick comments:
In `@backend/common-cdk/common_constructs/stack.py`:
- Around line 151-165: The ValueError in the allowed_origins property
incorrectly omits the ui_domain_name_override resolution and also allows an
empty-string override to produce an invalid "https://" origin; update the error
text to mention providing ui_domain_name_override (or its config key) as a valid
option, and change the ui_domain_name check (the property using
self.ui_domain_name) to require a truthy value (e.g., if self.ui_domain_name) so
empty strings are rejected and will trigger the ValueError; ensure references to
environment_context keys (allow_local_ui, local_ui_port) remain unchanged.
In `@backend/compact-connect/common_constructs/cc_api.py`:
- Around line 114-121: The current logic sets disable_execute_api_endpoint based
only on environment_name which can disable the API when no custom domain exists;
update the condition to only disable the default execute-api endpoint when
environment_name is in ('test','beta','prod') AND the stack has a hosted zone
(check stack.hosted_zone is not None) before passing
disable_execute_api_endpoint into super().__init__; refer to the variables
environment_name, stack.hosted_zone, disable_execute_api_endpoint and the
super().__init__ call to locate and change the logic.
In `@backend/cosmetology-app/docs/README.md`:
- Line 52: The example value for the licenseNumber field uses "OT12345" which is
for Occupational Therapy and may confuse readers; update the example in the
table row for "licenseNumber*" to a cosmetology-appropriate value (e.g.,
"COS12345") so it matches the FAQ CSV example and domain context; ensure the
table cell containing the example is replaced accordingly for the licenseNumber
entry.
In
`@backend/cosmetology-app/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py`:
- Around line 808-811: Update the misleading comment inside the if block that
checks len(provider_records_after_rollback.provider_records) > 1 in
rollback_license_upload.py: remove the stale word "privilege" and reword the
message to state we never expect multiple provider_records and that this is a
defensive guard against unexpected non-license/non-upload records (so the
comment accurately describes the check on
provider_records_after_rollback.provider_records).
In
`@backend/cosmetology-app/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py`:
- Around line 620-623: The pagination test currently asserts the same sort-key
SHA for mock_first_provider_id and mock_second_provider_id which is correct but
non-obvious; update the pagination test near the assertions for
mock_first_provider_id and mock_second_provider_id to add a brief comment
(similar to the existing note next to the single-provider assertion) explaining
that providerId is part of the DynamoDB partition key, not the sort key, so
identical update content produces the same SK/SHA — reference the specific
sort-key string/sha being asserted to make the rationale clear to future
readers.
In `@backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py`:
- Around line 21-26: Update the stale docstring on _common_search_request_schema
to remove the reference to the removed privilege search endpoints
(search_privileges_request_model / search_privileges_response_model) and instead
describe the schema as used by the provider search endpoint or as a
shared/common search request schema for pagination (e.g., "used by provider
search endpoint" or "used by search endpoints requiring search_after
pagination"); keep the link to OpenSearch DSL pagination and any other relevant
details intact.
In `@backend/cosmetology-app/tests/common_constructs/test_data_migration.py`:
- Around line 26-27: The test sets environment_name='sandbox' but the
StandardTags instance uses environment='test'; update the tags to match by
changing the StandardTags(...) call (the environment argument) from 'test' to
'sandbox' so environment_name and StandardTags.environment are consistent (look
for the StandardTags(...) instantiation in test_data_migration.py).
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to merge!
3 participants
The Cosmetology compact does not collect npi in their license records, and they intend to require all license records to include a license number as they will use this to search for particular practitioners in the system. This updates the schemas in the Cosmetology API to remove reference to npi and enforce the inclusion of the licenseNumber field with every license upload
The Cosmetology API will be called by the same UI as the original compact connect app used by JCC. As such,
we needed to set the explicit ui domain in the env context so we set the correct callback urls, CORS allowed origins, and
email template links. This adds that needed change so that CSV license uploads through the app will work correctly.
This PR also removes API endpoints for features that will not be applicable to the Cosmetology compact. This includes endpoints for programmatically reading privileges from the system, as well as the search endpoint for exporting privilege CSV reports.
Requirements List
Description List
Testing List
backend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #1290 #1283
Summary by CodeRabbit
New Features
Breaking Changes
Documentation