Determine if license is eligible/ineligible in the public search list view response#1558
Open
landonshumway-ia wants to merge 24 commits into
Open
Determine if license is eligible/ineligible in the public search list view response#1558landonshumway-ia wants to merge 24 commits into
landonshumway-ia wants to merge 24 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughDetect home-jurisdiction changes on license ingest, emit provider.homeStateChange events, and notify the former jurisdiction’s operations team via a new EmailNotificationService; update provider indexing, public search eligibility, infra wiring, and add unit/function/smoke tests. ChangesHome Jurisdiction Change Notification
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 7
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/stacks/api_stack/v1_api/api_model.py (1)
1492-1514:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
licenseEligibilityis missing from the public list response contract.Line 1492+ defines the public search list row schema, but it does not include the new
licenseEligibilityfield in eitherrequiredorproperties. That leaves the API model out of sync with the PR objective and can break frontend/API contract validation.Proposed schema update
def _public_license_search_response_schema(self): """Schema for public query providers response""" stack: AppStack = AppStack.of(self.api) return JsonSchema( type=JsonSchemaType.OBJECT, required=[ 'providerId', 'givenName', 'familyName', 'licenseJurisdiction', 'compact', 'licenseType', 'licenseNumber', + 'licenseEligibility', ], properties={ 'providerId': JsonSchema(type=JsonSchemaType.STRING, pattern=cc_api.UUID4_FORMAT), 'givenName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100), 'familyName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100), 'licenseJurisdiction': JsonSchema( type=JsonSchemaType.STRING, enum=stack.node.get_context('jurisdictions') ), 'compact': JsonSchema(type=JsonSchemaType.STRING, enum=stack.node.get_context('compacts')), 'licenseType': JsonSchema( type=JsonSchemaType.STRING, description='License type or profession designation for this license row', ), 'licenseNumber': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100), + 'licenseEligibility': JsonSchema( + type=JsonSchemaType.STRING, + enum=['eligible', 'ineligible'], + description='Eligibility status for this license row in public search', + ), }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py` around lines 1492 - 1514, The public search list row schema block (the JsonSchema that lists required=[...]) is missing the new licenseEligibility field; add "licenseEligibility" to the required list and add a corresponding properties entry for 'licenseEligibility' (e.g., a JsonSchema with type=JsonSchemaType.STRING, a short description like "license eligibility status for this license row", and sensible length/enum constraints consistent with other fields such as min_length=1 and max_length=100 or an enum if applicable) so the API model in api_model.py stays in sync with the PR objective.
🧹 Nitpick comments (15)
backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
469-525: 💤 Low valueOptional: Rename
test_find_best_license_*methods to reflect the renamed function.The test method names (
test_find_best_license_date_of_issuance_preferred_when_no_renewal,test_find_best_license_raises_exception_when_no_licenses,test_find_best_license_complex_scenario) still reference the oldfind_best_licensename even though they now callfind_most_recently_issued_or_renewed_license. Consider renaming for consistency with the new public API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py` around lines 469 - 525, The test method names still reference the old find_best_license name; update these to match the renamed public API by renaming test_find_best_license_date_of_issuance_preferred_when_no_renewal, test_find_best_license_raises_exception_when_no_licenses, and related test_find_best_license_* methods to use find_most_recently_issued_or_renewed_license (e.g., test_find_most_recently_issued_or_renewed_by_dateOfIssuance_when_no_renewal, test_find_most_recently_issued_or_renewed_raises_when_no_licenses, etc.) so test names reflect the function ProviderRecordUtility.find_most_recently_issued_or_renewed_license and maintain consistency.backend/compact-connect/tests/smoke/compact_configuration_smoke_tests.py (1)
106-116: 💤 Low valueUse
str | Nonefor the new optionalcompactparameters.
compact: str = Noneannotates the parameter asstrwhile allowingNoneas the default — the rest of this codebase consistently usesstr | None = Nonefor optional string parameters. Aligning these signatures keeps type-checkers happy and matches the style used elsewhere.♻️ Suggested diff
-def test_compact_configuration(compact: str = None): +def test_compact_configuration(compact: str | None = None):-def test_jurisdiction_configuration(compact: str = None, jurisdiction: str = 'ne', recreate_compact_config: bool = False): +def test_jurisdiction_configuration( + compact: str | None = None, jurisdiction: str = 'ne', recreate_compact_config: bool = False +):Also applies to: 226-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/compact-connect/tests/smoke/compact_configuration_smoke_tests.py` around lines 106 - 116, The parameter annotation for the optional compact argument in test_compact_configuration should use the union type used elsewhere: change the signature from compact: str = None to compact: str | None = None; update any other optional string parameters in the same file (the other test function(s) around the later occurrence) to follow the same pattern so type-checkers and code style remain consistent (look for the other function signatures mentioned in the comment and replace their `str = None` annotations with `str | None = None`).backend/cosmetology-app/lambdas/python/search/handlers/public_search.py (1)
47-80: ⚖️ Poor tradeoffEligibility helper performs a full provider-schema load per hit — verify performance and intent on validation failure.
A couple of concerns worth confirming:
ProviderOpenSearchDocumentSchema().load(provider_source)is invoked once per OpenSearch hit. For a full page of results this is potentially N full deep validations on data that the indexer already produced. If the per-hit cost is non-trivial, consider whether the eligibility check can be derived directly from the raw_source(e.g., inspectingadverseActionsandlicenses[0]['compactEligibility']directly), keeping the schema load as a fallback only when needed.- On
ValidationError, the helper currently returnsINELIGIBLE. This is a reasonable fail-closed default for the public-facing API, but it does mean any indexer/schema drift will silently surface every affected provider as "ineligible" rather than failing visibly. Thelogger.errormitigates this somewhat — please confirm that's the intended behavior (vs. e.g. raising and letting it propagate to a 500, since the upstreamtry/except ValidationErrorin_public_query_licenseswould then just skip the row).- Line 76 unconditionally indexes
licenses_list[0], which is safe given the indexing model (one license per provider doc), but_determine_license_eligibilityis called with the rawsourcewhile_public_query_licensesseparately validatessource.get('licenses')is non-empty on line 117. Worth a short docstring note that this helper assumes exactly one license entry (matching the indexing model) so future readers don't accidentally call it with multi-license sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/search/handlers/public_search.py` around lines 47 - 80, The helper _determine_license_eligibility currently does a full ProviderOpenSearchDocumentSchema().load(provider_source) per hit and returns INELIGIBLE on ValidationError; change it to first attempt a cheap, direct inspection of provider_source['adverseActions'] and provider_source.get('licenses', [])[0]['compactEligibility'] to decide eligibility (only invoking schema.load(...) as a fallback when those raw keys are missing or malformed), keep the fail-closed behavior but make it explicit by documenting in the function docstring that ValidationError intentionally maps to CompactEligibilityStatus.INELIGIBLE and that the function assumes exactly one license entry (indexing model), and retain the logger.error call so validation failures remain logged.backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.ts (1)
67-70: 💤 Low valueSubject line omits the new home state.
The subject is fixed at
Practitioner Home State Change - ${compactConfig.compactName}with no provider/state context. The recipient (former state ops team) needs to scan a queue of these — including the practitioner name and/or destination state in the subject (consistent with other notifications in this lambda like "License Encumbrance Notification - John Doe") would substantially improve triage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.ts` around lines 67 - 70, Update the subject construction in email-notification-service.ts (the subject variable that currently uses compactConfig.compactName) to include provider identity and the new home state; for example, build the subject using providerFirstName and providerLastName and formattedNewJurisdiction (e.g., `Practitioner Home State Change - ${providerFirstName} ${providerLastName} to ${formattedNewJurisdiction} - ${compactConfig.compactName}`) so recipients can triage messages from the inbox without opening the email.backend/cosmetology-app/lambdas/nodejs/tests/email-notification-service.test.ts (2)
1082-1094: 💤 Low valueTest fixture has
jurisdictionequal topreviousJurisdiction, masking a potential bug.Both
jurisdictionandpreviousJurisdictionare'tx'in the sample event. Becausehome_state_change_events.pyinvokes the lambda withjurisdiction=former_home_jurisdiction(i.e., they intentionally match in production), this fixture is realistic — but it also means the test cannot distinguish the case where the email service incorrectly substitutes one for the other. Consider asserting that the recipient lookup keyed offjurisdiction='tx'matchespreviousJurisdiction='tx'end-to-end, or alternatively settingpreviousJurisdictionto a different value so the body assertion on Line 1153 (from TX to OH) catches accidental cross-wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/nodejs/tests/email-notification-service.test.ts` around lines 1082 - 1094, The fixture SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT sets both jurisdiction and templateVariables.previousJurisdiction to 'tx', which masks cross-wiring bugs; update the fixture so previousJurisdiction differs (e.g., previousJurisdiction: 'tx' and jurisdiction: 'oh' or vice versa) OR add an explicit end-to-end assertion that the recipient lookup used the event.jurisdiction (former_home_jurisdiction) rather than templateVariables.previousJurisdiction—locate SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT and modify templateVariables.previousJurisdiction (or add the recipient lookup assertion around the email body check that expects "from TX to OH") so the test will fail if the service substitutes the wrong field.
1160-1170: ⚡ Quick winAdd a missing-jurisdiction test case for parity.
Other
State Notificationdescribe blocks in this file (e.g., License Encumbrance State Notification, lines 284–293; Privilege Encumbrance State Notification, lines 564–573) include a test asserting the handler throws whenjurisdictionisundefined. The newhomeJurisdictionChangeNotificationcase inlambda.ts(Lines 331–333) has the same guard but no corresponding test.🧪 Suggested test addition
it('should throw error when required template variables are missing', async () => { const eventWithMissingVariables: EmailNotificationEvent = { ...SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT, templateVariables: {} }; await expect(lambda.handler(eventWithMissingVariables, {} as any)) .rejects .toThrow('Missing required template variables for home jurisdiction change notification template.'); }); + + it('should throw error when jurisdiction is missing', async () => { + const eventWithMissingJurisdiction: EmailNotificationEvent = { + ...SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT, + jurisdiction: undefined + }; + + await expect(lambda.handler(eventWithMissingJurisdiction, {} as any)) + .rejects + .toThrow('Missing required jurisdiction field for home jurisdiction change notification template.'); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/nodejs/tests/email-notification-service.test.ts` around lines 1160 - 1170, Add a test that mirrors the other "State Notification" missing-jurisdiction cases: create a copy of SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT with jurisdiction set to undefined (or removed) and assert that calling lambda.handler(eventWithoutJurisdiction, {} as any) rejects with the same error thrown by the homeJurisdictionChangeNotification guard. Reference SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT and lambda.handler (and the homeJurisdictionChangeNotification guard behavior in lambda.ts) when adding the test to the existing describe block so the test suite covers the missing-jurisdiction path.backend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.py (2)
399-407: 💤 Low value
licenseTypeis sent in the payload but unused by the email lambda.The Python client populates
templateVariables.licenseType(Line 405), but the NodeJS handler case inlambda.ts(Lines 334–340) does not validate or readlicenseType, andEmailNotificationService.sendHomeJurisdictionChangeStateNotificationEmaildoes not accept or render it. This is either dead payload data or a missed inclusion in the email body. Decide whether to drop it from the Python payload or include the license type in the rendered email body/subject (which would also help recipients identify which credential moved when a practitioner holds multiple license types).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.py` around lines 399 - 407, The payload currently includes templateVariables.licenseType in email_service_client.py but the NodeJS path doesn't consume it; either remove this dead field from templateVariables in email_service_client.py, or extend the NodeJS flow to accept and render it: update the handler that reads the incoming payload (the case that routes to EmailNotificationService) to extract licenseType, add a parameter to EmailNotificationService.sendHomeJurisdictionChangeStateNotificationEmail to accept licenseType, and update the email template rendering logic to include the license type in subject/body; ensure all call sites and type signatures are updated accordingly.
41-51: 💤 Low valueType annotation inconsistent with runtime check.
provider_id: UUIDis declared non-Optional on the dataclass (Line 51), butsend_provider_home_state_change_emailchecksif template_variables.provider_id is None(Line 391). The check is unreachable for callers that respect the type, and a static type-checker will flag mypy/pyright errors on the assignment. This matches the pre-existing pattern inInvestigationNotificationTemplateVariables, so it isn't a regression — flagging only for awareness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.py` around lines 41 - 51, The dataclass HomeJurisdictionChangeNotificationTemplateVariables declares provider_id as UUID but the runtime code in send_provider_home_state_change_email checks for None; make the types consistent by changing provider_id: UUID to provider_id: Optional[UUID] (and add the Optional import) so the None check is valid, or alternatively remove the None check in send_provider_home_state_change_email; update the annotation to Optional[UUID] to match the existing pattern used by InvestigationNotificationTemplateVariables if you want to allow None.backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (1)
705-722: ⚡ Quick winHardcoded
providerIdin the asserted event detail is brittle.Line 712 hardcodes
'89a6377e-c3a5-40e5-bca5-317ec854c570'rather than using theprovider_idreturned fromself._with_ingested_license()at line 671. If the fixture (provider-ssn.json) ever changes, the entireDetailJSON string match will fail with a confusing diff. Reuse the existing variable for resilience.♻️ Suggested fix
'Detail': json.dumps( { 'compact': 'cosm', 'jurisdiction': 'ky', 'eventTime': '2024-11-08T23:59:59+00:00', - 'providerId': '89a6377e-c3a5-40e5-bca5-317ec854c570', + 'providerId': provider_id, 'licenseType': 'cosmetologist', 'formerHomeJurisdiction': 'oh', } ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py` around lines 705 - 722, The asserted event detail is brittle because it hardcodes the provider UUID instead of using the provider_id returned by self._with_ingested_license(); update the expected object used in the assertion (the dict compared to home_change_entry) to reference the provider_id variable (from the self._with_ingested_license() call) for the 'providerId' value and ensure the json.dumps call uses that provider_id so the test remains resilient to fixture changes.backend/cosmetology-app/lambdas/python/common/tests/unit/test_email_service_client.py (1)
49-68: 💤 Low valueConsider comparing parsed JSON instead of exact
json.dumpsstring.Asserting on
Payload=json.dumps({...})couples the test to a specific dict iteration order in the production code. If the implementation ever changes the key order in its serialization (or switchesjson.dumpsoptions), this assertion breaks despite the payload being semantically identical. A more resilient approach extracts thePayloadkwarg, decodes it, and compares parsed dicts.♻️ Suggested approach
- mock_lambda_client.invoke.assert_called_once_with( - FunctionName='test-lambda-name', - InvocationType='RequestResponse', - Payload=json.dumps( - { - 'compact': TEST_COMPACT, - 'jurisdiction': TEST_FORMER_JURISDICTION, - 'template': 'homeJurisdictionChangeNotification', - 'recipientType': 'JURISDICTION_OPERATIONS_TEAM', - 'templateVariables': { - 'providerFirstName': 'Jane', - 'providerLastName': 'Smith', - 'providerId': str(TEST_PROVIDER_ID), - 'previousJurisdiction': TEST_FORMER_JURISDICTION, - 'newJurisdiction': TEST_NEW_JURISDICTION, - 'licenseType': 'Cosmetologist', - }, - } - ), - ) + mock_lambda_client.invoke.assert_called_once() + call_kwargs = mock_lambda_client.invoke.call_args.kwargs + self.assertEqual('test-lambda-name', call_kwargs['FunctionName']) + self.assertEqual('RequestResponse', call_kwargs['InvocationType']) + self.assertEqual( + { + 'compact': TEST_COMPACT, + 'jurisdiction': TEST_FORMER_JURISDICTION, + 'template': 'homeJurisdictionChangeNotification', + 'recipientType': 'JURISDICTION_OPERATIONS_TEAM', + 'templateVariables': { + 'providerFirstName': 'Jane', + 'providerLastName': 'Smith', + 'providerId': str(TEST_PROVIDER_ID), + 'previousJurisdiction': TEST_FORMER_JURISDICTION, + 'newJurisdiction': TEST_NEW_JURISDICTION, + 'licenseType': 'Cosmetologist', + }, + }, + json.loads(call_kwargs['Payload']), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/common/tests/unit/test_email_service_client.py` around lines 49 - 68, Update the test to avoid asserting on the exact json.dumps string: grab the actual call to mock_lambda_client.invoke (e.g. via mock_lambda_client.invoke.call_args or call_args_list), extract the Payload kwarg, decode it with json.loads, and compare the resulting dict to the expected dict (including nested templateVariables) instead of comparing the raw serialized string; keep the existing assertions for FunctionName and InvocationType but replace the Payload equality check with a parsed-dict comparison.backend/cosmetology-app/tests/smoke/smoke_common.py (2)
438-464: 💤 Low valueDocument precedence of
jurisdictionsoverjurisdiction.When both are provided,
jurisdictionssilently wins (line 457). That's a reasonable default but isn't reflected in the docstring, which could confuse callers debugging a wrong scope set. Consider noting the precedence in the docstring (or rejecting the case where both are supplied).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/tests/smoke/smoke_common.py` around lines 438 - 464, The docstring for create_test_app_client does not state that the jurisdictions parameter takes precedence over jurisdiction when both are provided; update the docstring for create_test_app_client to explicitly document that if both jurisdictions (list) and jurisdiction (single) are supplied, jurisdictions will be used to build allowed_scopes (or alternatively implement validation to reject both), so callers know the precedence of jurisdictions over jurisdiction and how allowed_scopes is derived.
387-405: 💤 Low valueEdge case:
max_attemptscan be0ifpoll_interval_seconds > max_wait_time.With
max_wait_time=300(default) the math works out fine, but if a caller passes apoll_interval_secondslarger thanmax_wait_time(e.g.max_wait_time=30, poll_interval_seconds=60),max_attemptsbecomes0and the function raisesSmokeTestFailureExceptionimmediately without ever polling. Either guard withmax(1, ...)or document the constraint in the docstring.♻️ Suggested fix
- max_attempts = max_wait_time // poll_interval_seconds + max_attempts = max(1, max_wait_time // poll_interval_seconds)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/tests/smoke/smoke_common.py` around lines 387 - 405, The code computes max_attempts as "max_wait_time // poll_interval_seconds" which can yield 0 when poll_interval_seconds > max_wait_time, causing the loop to skip polling; change the computation of max_attempts to ensure at least one attempt (e.g., max_attempts = max(1, max_wait_time // poll_interval_seconds)) or otherwise clamp poll_interval_seconds, so the while loop using attempts < max_attempts will always perform at least one call to query_provider_by_name (refer to max_attempts, poll_interval_seconds, max_wait_time and the polling loop that calls get_staff_user_auth_headers/query_provider_by_name).backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py (1)
97-98: 💤 Low valueRenamed method still seeds a registered provider — name is now slightly less precise.
The body still seeds
compactConnectRegisteredEmailAddress(line 104) so the test specifically exercises the "registered provider" path. Dropping "registered" from the name and docstring makes it ambiguous compared with neighboring tests liketest_license_investigation_listener_handles_missing_provider_records. Consider keeping "registered" or adding a brief docstring note clarifying the seeded state. Same applies to lines 161, 222, and 283.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py` around lines 97 - 98, The test method test_license_investigation_listener_processes_event_with_provider currently seeds compactConnectRegisteredEmailAddress (a registered provider) but its name/docs drop "registered", causing ambiguity; either rename the test to include "registered" (e.g., test_license_investigation_listener_processes_event_with_registered_provider) or update the docstring to explicitly state that the test seeds a registered provider, and apply the same clarity fix to the other similar tests referenced around lines 161, 222, and 283 so their names/docs match the seeded state.backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py (2)
220-227: 💤 Low valueInconsistent
max_wait_secondsbetween caller and default.
_wait_for_home_state_change_eventdefaults to720seconds (line 112), but the only caller passes750(line 222). Either align the default or drop the explicit override to keep the values in sync. Same minor inconsistency exists betweenwait_for_provider_creation(max_wait_time=750, ...)(line 175) and the helper's default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py` around lines 220 - 227, Tests pass an explicit max_wait_seconds=750 to _wait_for_home_state_change_event and wait_for_provider_creation while the helpers default to 720; make these consistent by updating the helper defaults to 750 (change the default parameter in _wait_for_home_state_change_event and in wait_for_provider_creation to 750) so callers can omit the explicit override and the values remain in sync.
112-137: 💤 Low value
FilterExpressionis applied after pagination — may miss events on a busy table.
_wait_for_home_state_change_eventruns a singlequeryper attempt and appliesFilterExpression='providerId = :provider_id'afterwards. In DynamoDB, the filter is evaluated after the page is read but before results are returned, so if the matching event sits beyond the first page (e.g. many concurrent events on the samepk), it can be silently skipped this attempt. The polling loop will likely catch it on a later attempt, but pagination would be more robust.In practice this smoke test runs against a sandbox with little contention, so it's a minor reliability concern rather than a bug.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py` around lines 112 - 137, The query in _wait_for_home_state_change_event uses FilterExpression='providerId = :provider_id' which is applied client-side per page and can miss matches on subsequent pages; instead, inside each attempt iterate through all query pages (use response.get('LastEvaluatedKey') / ExclusiveStartKey) for the KeyConditionExpression f'COMPACT#{COMPACT}#JURISDICTION#{HOME_STATE_CHANGE_NEW_JURISDICTION}' and scan each page's Items for an item whose 'providerId' equals the provider_id argument; stop and return the matching_event if found, otherwise continue pagination until exhausted, then sleep and retry the outer attempt loop—keep ConsistentRead=True and the existing logging around provider.homeStateChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/compact-connect/lambdas/python/data-events/requirements-dev.txt`:
- Line 61: The PR pins urllib3==2.7.0 which drops Python 3.9/PyPy3.10 support
and requires pyOpenSSL>=19.0.0; before merging, verify the CI/dev environment
and any consumers support Python >=3.10 (or update target interpreters), add a
note in requirements-dev.txt or project README documenting the Python minimum,
and ensure pyOpenSSL is upgraded (add/raise its version in requirements-dev.txt
or a constraints file) so builds and tests remain green.
In
`@backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.ts`:
- Around line 52-60: The current code assumes getJurisdictionRecipients returns
an array but if jurisdictionConfig.jurisdictionOperationsTeamEmails is missing
it can return undefined causing a TypeError on recipients.length; update the
call-site or the getJurisdictionRecipients implementation so recipients is
always an array (e.g., coerce undefined to [] or return [] from
getJurisdictionRecipients) and then keep the existing check (change the
condition to check for an empty array or falsy/length: recipients = recipients
?? []; if (recipients.length === 0) throw ...). Ensure references:
getJurisdictionRecipients, recipients, and
jurisdictionConfig.jurisdictionOperationsTeamEmails are the places to adjust.
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.py`:
- Around line 145-146: The handlers in investigation_events.py call
config.data_client.get_provider_top_level_record(...) directly (at the four
sites around lines 146, 213, 274, 341) and thus miss the error-catching/logging
behavior that encumbrance_events.py enforces via its _get_provider_records(...)
wrapper; update investigation_events.py to either call the existing
_get_provider_records(...) helper (or add an equivalent wrapper in
investigation_events.py) so that calls to get_provider_top_level_record are
wrapped to catch exceptions, log failures with processLogger.error (or the
module's logger) and then re-raise, ensuring consistent error visibility for
`@sqs_handler-decorated` handlers.
In
`@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_home_state_change_events.py`:
- Around line 22-23: Update the stale class docstring on
TestHomeStateChangeEvents to accurately describe this test suite (it covers home
state change event handlers, not investigation event handlers); locate the
TestHomeStateChangeEvents class and replace the docstring "Test suite for
investigation event handlers." with a concise description like "Test suite for
home state change event handlers." to reflect its purpose.
- Around line 46-47: Method name has a typo: rename
test_license_homes_state_change_listener_sends_notification_to_former_state to
test_license_home_state_change_listener_sends_notification_to_former_state to
match the docstring and project naming (align with
home_state_change_notification_listener); update any references or test
discovery expectations that use the old name so the test still runs and keep the
docstring unchanged.
In `@backend/cosmetology-app/lambdas/python/search/handlers/public_search.py`:
- Around line 40-44: The tests lack explicit coverage that privilege-level
adverse actions are included in the top-level adverseActions field of OpenSearch
documents; add a new test method in TestGenerateOpenSearchDocuments that uses
get_adverse_action_records() (or the existing test fixtures used by
TestGenerateOpenSearchDocuments) to create at least one privilege-level adverse
action record, call the document generation routine that produces docs (the same
flow used by existing tests like test_license_adverse_actions_included), and
assert that docs[0]['adverseActions'] contains the privilege-level record; also
update or rename any misleading variables like provider_level_adverse_actions in
the test to reflect that adverseActions aggregates provider, license, and
privilege records so the intent is clear.
In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py`:
- Around line 353-354: The error log in the except block for
SmokeTestFailureException still says "License record upload" but the test flow
was renamed; update the logger.error call inside the except
SmokeTestFailureException handler (in license_upload_smoke_tests.py) to use the
current test name by replacing the message string with the new flow name (e.g.,
"license upload smoke test failed" or the exact renamed flow) so the log matches
the test being run while preserving inclusion of str(e).
---
Outside diff comments:
In `@backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py`:
- Around line 1492-1514: The public search list row schema block (the JsonSchema
that lists required=[...]) is missing the new licenseEligibility field; add
"licenseEligibility" to the required list and add a corresponding properties
entry for 'licenseEligibility' (e.g., a JsonSchema with
type=JsonSchemaType.STRING, a short description like "license eligibility status
for this license row", and sensible length/enum constraints consistent with
other fields such as min_length=1 and max_length=100 or an enum if applicable)
so the API model in api_model.py stays in sync with the PR objective.
---
Nitpick comments:
In `@backend/compact-connect/tests/smoke/compact_configuration_smoke_tests.py`:
- Around line 106-116: The parameter annotation for the optional compact
argument in test_compact_configuration should use the union type used elsewhere:
change the signature from compact: str = None to compact: str | None = None;
update any other optional string parameters in the same file (the other test
function(s) around the later occurrence) to follow the same pattern so
type-checkers and code style remain consistent (look for the other function
signatures mentioned in the comment and replace their `str = None` annotations
with `str | None = None`).
In
`@backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.ts`:
- Around line 67-70: Update the subject construction in
email-notification-service.ts (the subject variable that currently uses
compactConfig.compactName) to include provider identity and the new home state;
for example, build the subject using providerFirstName and providerLastName and
formattedNewJurisdiction (e.g., `Practitioner Home State Change -
${providerFirstName} ${providerLastName} to ${formattedNewJurisdiction} -
${compactConfig.compactName}`) so recipients can triage messages from the inbox
without opening the email.
In
`@backend/cosmetology-app/lambdas/nodejs/tests/email-notification-service.test.ts`:
- Around line 1082-1094: The fixture
SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT sets both
jurisdiction and templateVariables.previousJurisdiction to 'tx', which masks
cross-wiring bugs; update the fixture so previousJurisdiction differs (e.g.,
previousJurisdiction: 'tx' and jurisdiction: 'oh' or vice versa) OR add an
explicit end-to-end assertion that the recipient lookup used the
event.jurisdiction (former_home_jurisdiction) rather than
templateVariables.previousJurisdiction—locate
SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT and modify
templateVariables.previousJurisdiction (or add the recipient lookup assertion
around the email body check that expects "from TX to OH") so the test will fail
if the service substitutes the wrong field.
- Around line 1160-1170: Add a test that mirrors the other "State Notification"
missing-jurisdiction cases: create a copy of
SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT with jurisdiction
set to undefined (or removed) and assert that calling
lambda.handler(eventWithoutJurisdiction, {} as any) rejects with the same error
thrown by the homeJurisdictionChangeNotification guard. Reference
SAMPLE_HOME_JURISDICTION_CHANGE_NEW_STATE_NOTIFICATION_EVENT and lambda.handler
(and the homeJurisdictionChangeNotification guard behavior in lambda.ts) when
adding the test to the existing describe block so the test suite covers the
missing-jurisdiction path.
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.py`:
- Around line 399-407: The payload currently includes
templateVariables.licenseType in email_service_client.py but the NodeJS path
doesn't consume it; either remove this dead field from templateVariables in
email_service_client.py, or extend the NodeJS flow to accept and render it:
update the handler that reads the incoming payload (the case that routes to
EmailNotificationService) to extract licenseType, add a parameter to
EmailNotificationService.sendHomeJurisdictionChangeStateNotificationEmail to
accept licenseType, and update the email template rendering logic to include the
license type in subject/body; ensure all call sites and type signatures are
updated accordingly.
- Around line 41-51: The dataclass
HomeJurisdictionChangeNotificationTemplateVariables declares provider_id as UUID
but the runtime code in send_provider_home_state_change_email checks for None;
make the types consistent by changing provider_id: UUID to provider_id:
Optional[UUID] (and add the Optional import) so the None check is valid, or
alternatively remove the None check in send_provider_home_state_change_email;
update the annotation to Optional[UUID] to match the existing pattern used by
InvestigationNotificationTemplateVariables if you want to allow None.
In
`@backend/cosmetology-app/lambdas/python/common/tests/unit/test_email_service_client.py`:
- Around line 49-68: Update the test to avoid asserting on the exact json.dumps
string: grab the actual call to mock_lambda_client.invoke (e.g. via
mock_lambda_client.invoke.call_args or call_args_list), extract the Payload
kwarg, decode it with json.loads, and compare the resulting dict to the expected
dict (including nested templateVariables) instead of comparing the raw
serialized string; keep the existing assertions for FunctionName and
InvocationType but replace the Payload equality check with a parsed-dict
comparison.
In
`@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py`:
- Around line 469-525: The test method names still reference the old
find_best_license name; update these to match the renamed public API by renaming
test_find_best_license_date_of_issuance_preferred_when_no_renewal,
test_find_best_license_raises_exception_when_no_licenses, and related
test_find_best_license_* methods to use
find_most_recently_issued_or_renewed_license (e.g.,
test_find_most_recently_issued_or_renewed_by_dateOfIssuance_when_no_renewal,
test_find_most_recently_issued_or_renewed_raises_when_no_licenses, etc.) so test
names reflect the function
ProviderRecordUtility.find_most_recently_issued_or_renewed_license and maintain
consistency.
In
`@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py`:
- Around line 97-98: The test method
test_license_investigation_listener_processes_event_with_provider currently
seeds compactConnectRegisteredEmailAddress (a registered provider) but its
name/docs drop "registered", causing ambiguity; either rename the test to
include "registered" (e.g.,
test_license_investigation_listener_processes_event_with_registered_provider) or
update the docstring to explicitly state that the test seeds a registered
provider, and apply the same clarity fix to the other similar tests referenced
around lines 161, 222, and 283 so their names/docs match the seeded state.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py`:
- Around line 705-722: The asserted event detail is brittle because it hardcodes
the provider UUID instead of using the provider_id returned by
self._with_ingested_license(); update the expected object used in the assertion
(the dict compared to home_change_entry) to reference the provider_id variable
(from the self._with_ingested_license() call) for the 'providerId' value and
ensure the json.dumps call uses that provider_id so the test remains resilient
to fixture changes.
In `@backend/cosmetology-app/lambdas/python/search/handlers/public_search.py`:
- Around line 47-80: The helper _determine_license_eligibility currently does a
full ProviderOpenSearchDocumentSchema().load(provider_source) per hit and
returns INELIGIBLE on ValidationError; change it to first attempt a cheap,
direct inspection of provider_source['adverseActions'] and
provider_source.get('licenses', [])[0]['compactEligibility'] to decide
eligibility (only invoking schema.load(...) as a fallback when those raw keys
are missing or malformed), keep the fail-closed behavior but make it explicit by
documenting in the function docstring that ValidationError intentionally maps to
CompactEligibilityStatus.INELIGIBLE and that the function assumes exactly one
license entry (indexing model), and retain the logger.error call so validation
failures remain logged.
In `@backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py`:
- Around line 220-227: Tests pass an explicit max_wait_seconds=750 to
_wait_for_home_state_change_event and wait_for_provider_creation while the
helpers default to 720; make these consistent by updating the helper defaults to
750 (change the default parameter in _wait_for_home_state_change_event and in
wait_for_provider_creation to 750) so callers can omit the explicit override and
the values remain in sync.
- Around line 112-137: The query in _wait_for_home_state_change_event uses
FilterExpression='providerId = :provider_id' which is applied client-side per
page and can miss matches on subsequent pages; instead, inside each attempt
iterate through all query pages (use response.get('LastEvaluatedKey') /
ExclusiveStartKey) for the KeyConditionExpression
f'COMPACT#{COMPACT}#JURISDICTION#{HOME_STATE_CHANGE_NEW_JURISDICTION}' and scan
each page's Items for an item whose 'providerId' equals the provider_id
argument; stop and return the matching_event if found, otherwise continue
pagination until exhausted, then sleep and retry the outer attempt loop—keep
ConsistentRead=True and the existing logging around provider.homeStateChange.
In `@backend/cosmetology-app/tests/smoke/smoke_common.py`:
- Around line 438-464: The docstring for create_test_app_client does not state
that the jurisdictions parameter takes precedence over jurisdiction when both
are provided; update the docstring for create_test_app_client to explicitly
document that if both jurisdictions (list) and jurisdiction (single) are
supplied, jurisdictions will be used to build allowed_scopes (or alternatively
implement validation to reject both), so callers know the precedence of
jurisdictions over jurisdiction and how allowed_scopes is derived.
- Around line 387-405: The code computes max_attempts as "max_wait_time //
poll_interval_seconds" which can yield 0 when poll_interval_seconds >
max_wait_time, causing the loop to skip polling; change the computation of
max_attempts to ensure at least one attempt (e.g., max_attempts = max(1,
max_wait_time // poll_interval_seconds)) or otherwise clamp
poll_interval_seconds, so the while loop using attempts < max_attempts will
always perform at least one call to query_provider_by_name (refer to
max_attempts, poll_interval_seconds, max_wait_time and the polling loop that
calls get_staff_user_auth_headers/query_provider_by_name).
🪄 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: c71e096f-0226-409f-9fef-0a499dffaa13
⛔ Files ignored due to path filters (2)
backend/compact-connect/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockbackend/cosmetology-app/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (75)
backend/compact-connect/lambdas/nodejs/package.jsonbackend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txtbackend/compact-connect/lambdas/python/common/requirements-dev.txtbackend/compact-connect/lambdas/python/common/requirements.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txtbackend/compact-connect/lambdas/python/custom-resources/requirements-dev.txtbackend/compact-connect/lambdas/python/data-events/requirements-dev.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txtbackend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/feature-flag/requirements.txtbackend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txtbackend/compact-connect/lambdas/python/purchases/requirements-dev.inbackend/compact-connect/lambdas/python/purchases/requirements-dev.txtbackend/compact-connect/lambdas/python/purchases/requirements.inbackend/compact-connect/lambdas/python/purchases/requirements.txtbackend/compact-connect/lambdas/python/search/requirements-dev.txtbackend/compact-connect/lambdas/python/search/requirements.txtbackend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txtbackend/compact-connect/lambdas/python/staff-users/requirements-dev.txtbackend/compact-connect/requirements-dev.txtbackend/compact-connect/tests/smoke/compact_configuration_smoke_tests.pybackend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.pybackend/cosmetology-app/lambdas/nodejs/email-notification-service/lambda.tsbackend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.tsbackend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.tsbackend/cosmetology-app/lambdas/nodejs/lib/email/index.tsbackend/cosmetology-app/lambdas/nodejs/package.jsonbackend/cosmetology-app/lambdas/nodejs/tests/email-notification-service.test.tsbackend/cosmetology-app/lambdas/python/cognito-backup/requirements-dev.txtbackend/cosmetology-app/lambdas/python/cognito-backup/requirements.txtbackend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/api.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/cosmetology-app/lambdas/python/common/cc_common/email_service_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/event_bus_client.pybackend/cosmetology-app/lambdas/python/common/requirements-dev.txtbackend/cosmetology-app/lambdas/python/common/requirements.txtbackend/cosmetology-app/lambdas/python/common/tests/unit/test_email_service_client.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.pybackend/cosmetology-app/lambdas/python/compact-configuration/requirements-dev.txtbackend/cosmetology-app/lambdas/python/custom-resources/requirements-dev.txtbackend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/handlers/home_state_change_events.pybackend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.pybackend/cosmetology-app/lambdas/python/data-events/requirements-dev.txtbackend/cosmetology-app/lambdas/python/data-events/tests/function/test_home_state_change_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.pybackend/cosmetology-app/lambdas/python/disaster-recovery/requirements-dev.txtbackend/cosmetology-app/lambdas/python/feature-flag/requirements-dev.txtbackend/cosmetology-app/lambdas/python/feature-flag/requirements.txtbackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.pybackend/cosmetology-app/lambdas/python/provider-data-v1/requirements-dev.txtbackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.pybackend/cosmetology-app/lambdas/python/search/handlers/public_search.pybackend/cosmetology-app/lambdas/python/search/opensearch_client.pybackend/cosmetology-app/lambdas/python/search/requirements-dev.txtbackend/cosmetology-app/lambdas/python/search/requirements.txtbackend/cosmetology-app/lambdas/python/search/tests/function/test_manage_opensearch_indices.pybackend/cosmetology-app/lambdas/python/search/tests/function/test_populate_provider_documents.pybackend/cosmetology-app/lambdas/python/search/tests/function/test_provider_update_ingest.pybackend/cosmetology-app/lambdas/python/search/tests/function/test_public_search_providers.pybackend/cosmetology-app/lambdas/python/staff-user-pre-token/requirements-dev.txtbackend/cosmetology-app/lambdas/python/staff-user-pre-token/tests/test_user_scopes.pybackend/cosmetology-app/lambdas/python/staff-user-pre-token/user_data.pybackend/cosmetology-app/lambdas/python/staff-users/requirements-dev.txtbackend/cosmetology-app/requirements-dev.txtbackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/notification_stack.pybackend/cosmetology-app/tests/app/test_notification_stack.pybackend/cosmetology-app/tests/smoke/license_upload_smoke_tests.pybackend/cosmetology-app/tests/smoke/smoke_common.pybackend/multi-account/README.mdbackend/multi-account/backups/requirements-dev.txtbackend/multi-account/control-tower/requirements-dev.txt
💤 Files with no reviewable changes (1)
- backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json (1)
343-348: ⚡ Quick winConsider adding maxItems constraint to error arrays.
The nested error message arrays lack a
maxItemsconstraint, which could allow unbounded arrays in error responses.Based on learnings, the static analysis tool Checkov flagged this. Consider adding a reasonable limit:
🛡️ Add maxItems constraint
"items": { + "maxItems": 100, "type": "array", "description": "List of error messages for a field", "items": { "type": "string" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json` around lines 343 - 348, The OpenAPI schema has nested error message arrays without a maxItems constraint; locate the error array schemas near keys like "licenseType" and "providerId" and add a reasonable "maxItems" (e.g., 50 or 100) to each array schema that models error lists (commonly named "errors", "errorMessages", "messages" or similar) so arrays cannot be unbounded; ensure you add the "maxItems" property alongside the existing "type": "array" definitions and keep the numeric cap consistent across all error-list schemas.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cosmetology-app/docs/api-specification/latest-oas30.json`:
- Line 5: The PR modified the wrong OpenAPI file (the State license-management
spec) instead of the Search API; locate the API spec that defines the
/v1/compacts/{compact}/providers/search endpoint and add the new
licenseEligibility field to the provider list response schema (e.g., the
provider compact item schema or the search response schema and its example),
update components/schemas and any examples to include licenseEligibility with
its type and description, and remove/revert the unintended change from the State
API spec so only the search-internal specification contains the new field.
---
Nitpick comments:
In
`@backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json`:
- Around line 343-348: The OpenAPI schema has nested error message arrays
without a maxItems constraint; locate the error array schemas near keys like
"licenseType" and "providerId" and add a reasonable "maxItems" (e.g., 50 or 100)
to each array schema that models error lists (commonly named "errors",
"errorMessages", "messages" or similar) so arrays cannot be unbounded; ensure
you add the "maxItems" property alongside the existing "type": "array"
definitions and keep the numeric cap consistent across all error-list schemas.
🪄 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: ec6bf3ff-23d8-4f0e-9f7c-7280d299eb10
📒 Files selected for processing (12)
backend/compact-connect/tests/smoke/compact_configuration_smoke_tests.pybackend/cosmetology-app/docs/api-specification/latest-oas30.jsonbackend/cosmetology-app/docs/internal/api-specification/latest-oas30.jsonbackend/cosmetology-app/docs/internal/postman/postman-collection.jsonbackend/cosmetology-app/docs/postman/postman-collection.jsonbackend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.jsonbackend/cosmetology-app/docs/search-internal/postman/postman-collection.jsonbackend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.pybackend/cosmetology-app/lambdas/python/search/handlers/public_search.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/tests/resources/snapshots/PUBLIC_QUERY_PROVIDERS_RESPONSE_SCHEMA.json
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py
- backend/cosmetology-app/lambdas/python/search/handlers/public_search.py
- backend/compact-connect/tests/smoke/compact_configuration_smoke_tests.py
| "info": { | ||
| "title": "StateApi", | ||
| "version": "2026-03-13T17:32:08Z" | ||
| "version": "2026-02-16T16:53:09Z" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the public search API specification
echo "=== Finding all API specifications ==="
fd 'latest-oas' backend/cosmetology-app/docs
echo -e "\n=== Check for public search endpoints in each spec ==="
for spec in $(fd 'latest-oas.*json' backend/cosmetology-app/docs); do
echo "=== $spec ==="
jq -r '.paths | keys[]' "$spec" 2>/dev/null | grep -i search || echo "No search endpoints"
done
echo -e "\n=== Search for GET endpoints that return provider/license lists ==="
rg -n '"get":|GET' backend/cosmetology-app/docs --type json -A10 | rg -A10 'provider|license' | head -50Repository: csg-org/CompactConnect
Length of output: 7731
🏁 Script executed:
#!/bin/bash
# Check the search API specification for licenseEligibility field
echo "=== Checking search-internal API spec ==="
jq -r '.components.schemas | keys[]' backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json 2>/dev/null | head -20
echo -e "\n=== Check for licenseEligibility field in search API ==="
grep -n "licenseEligibility" backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json || echo "Not found in search-internal spec"
echo -e "\n=== Version timestamp in search-internal spec ==="
head -10 backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json | jq '.info.version'
echo -e "\n=== Get provider-related response schemas in search API ==="
jq '.paths[] | .get.responses."200".content."application/json".schema' backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json 2>/dev/null | head -50Repository: csg-org/CompactConnect
Length of output: 443
🏁 Script executed:
#!/bin/bash
# Check the file validity and structure
echo "=== File info for search-internal spec ==="
wc -l backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json
stat backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json | grep -E "Size|Modify"
echo -e "\n=== Last 20 lines of search-internal spec ==="
tail -20 backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json
echo -e "\n=== First 20 lines of search-internal spec ==="
head -20 backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json
echo -e "\n=== Check if both specs have same timestamp ==="
echo "State API (license mgmt):"
head -10 backend/cosmetology-app/docs/api-specification/latest-oas30.json | grep version
echo -e "\nSearch Internal API:"
head -10 backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json | grep versionRepository: csg-org/CompactConnect
Length of output: 1540
🏁 Script executed:
#!/bin/bash
# Check the search API response schema
echo "=== Search API response schema ==="
jq '.paths["/v1/compacts/{compact}/providers/search"].post.responses."200".content."application/json".schema' backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json 2>/dev/null
echo -e "\n=== Search for licenseEligibility anywhere in search API ==="
grep -c "licenseEligibility" backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json && echo "Found licenseEligibility" || echo "licenseEligibility NOT found"
echo -e "\n=== Check license-related schemas in search API ==="
jq '.components.schemas | keys[] | select(contains("License") or contains("license"))' backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json 2>/dev/nullRepository: csg-org/CompactConnect
Length of output: 307
This file is the State API specification for license management, not the search API that handles public provider listings.
The file under review (backend/cosmetology-app/docs/api-specification/latest-oas30.json) is the State API specification for license management endpoints (POST licenses, GET bulk-upload). The PR objective requires updating the public search API response to add a licenseEligibility field. The correct API specification is backend/cosmetology-app/docs/search-internal/api-specification/latest-oas30.json, which contains the /v1/compacts/{compact}/providers/search endpoint that handles provider list-view responses. Verify that the correct API file has been modified per the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cosmetology-app/docs/api-specification/latest-oas30.json` at line 5,
The PR modified the wrong OpenAPI file (the State license-management spec)
instead of the Search API; locate the API spec that defines the
/v1/compacts/{compact}/providers/search endpoint and add the new
licenseEligibility field to the provider list response schema (e.g., the
provider compact item schema or the search response schema and its example),
update components/schemas and any examples to include licenseEligibility with
its type and description, and remove/revert the unintended change from the State
API spec so only the search-internal specification contains the new field.
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.
For the cosmetology public license list view, the frontend will be adding a column that shows if the license for that row is eligible or ineligible. This updates the API for the respective search endpoint to return a new field 'licenseEligibility', which the frontend will use to populate that column for the list view.
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsbackend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #1479
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores