Only generate cosmetology privilege records at runtime#1299
Open
landonshumway-ia wants to merge 21 commits intocsg-org:mainfrom
Open
Only generate cosmetology privilege records at runtime#1299landonshumway-ia wants to merge 21 commits intocsg-org:mainfrom
landonshumway-ia wants to merge 21 commits intocsg-org:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves persisted privilege records and schemas, generates privileges at API runtime from licenses and compact configuration, adds cached _Config.live_compact_jurisdictions, updates handlers/infra to read compact configuration, and refactors tests and event flows to operate from license + live-compact data. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Event Source
participant Handler as Lambda Handler
participant Config as _Config.live_compact_jurisdictions
participant Data as DataClient / Dynamo
participant EventBus as EventBusClient / Emailer
Event->>Handler: encumbrance / investigation event
Handler->>Config: read live_compact_jurisdictions(compact)
Config-->>Handler: { compact: [jurisdictions] }
Handler->>Data: fetch provider license records & adverse actions/investigations
Data-->>Handler: license records + adverse actions/investigations
Handler->>Handler: generate privileges from licenses (runtime)
Handler->>EventBus: publish notifications/events to live jurisdictions (excluding home)
EventBus-->>Handler: publish ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
We don't need to encumber or lift encumbrances for privileges downstream when a home state license is encumbered, since we do not store privilege records in the db under this model.
We do not deactivate privileges downstream under this model, so this listener can be removed to simplify the code base
landonshumway-ia
changed the title
![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: 4
🧹 Nitpick comments (6)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py (2)
78-79: Update stale inline comment to match the returned object.The comment says “test privilege record” but the function returns
license_record.🧹 Proposed comment fix
- # return both the test event and the test privilege record + # return both the test event and the test license record🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py` around lines 78 - 79, Update the stale inline comment that says “test privilege record” to accurately describe the returned object as `license_record`; locate the comment near the return statement that returns `test_event, license_record` (referencing the variables `test_event` and `license_record`) and change the wording to something like “return both the test event and the license record” so the comment matches the actual returned value.
55-57: UseDEFAULT_PRIVILEGE_JURISDICTIONin live-jurisdiction test setup.Line 57 and Line 403 hard-code
'ne', while the same tests already rely onDEFAULT_PRIVILEGE_JURISDICTIONelsewhere. Reusing the constant avoids future drift.♻️ Proposed refactor
- self.set_live_compact_jurisdictions_for_test({'cosm': ['ne']}) + self.set_live_compact_jurisdictions_for_test({'cosm': [DEFAULT_PRIVILEGE_JURISDICTION]})Also applies to: 401-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py` around lines 55 - 57, Replace hard-coded 'ne' with the shared DEFAULT_PRIVILEGE_JURISDICTION constant in the test setup and other affected test lines: update the setUp() call that uses set_live_compact_jurisdictions_for_test({'cosm': ['ne']}) to use DEFAULT_PRIVILEGE_JURISDICTION instead, and make the same substitution at the other occurrence around the investigation tests (the block referencing 'ne' at the end of the file). This ensures tests use the shared DEFAULT_PRIVILEGE_JURISDICTION symbol rather than a literal string.backend/cosmetology-app/stacks/api_lambda_stack/provider_management.py (1)
107-107: Refresh IAM suppression text to match the new table access scope.With compact configuration read grants added, the nearby
AwsSolutions-IAM5reason text is now slightly outdated. Updating it will keep audit context accurate.✏️ Suggested text-only update
- 'and is scoped to one table and encryption key.', + 'and is scoped to the provider table, compact configuration table, and encryption key.', ... - 'reason': 'The wildcard actions in this policy are scoped to the rate-limiting table and ' - 'the provider data table.', + 'reason': 'The wildcard actions in this policy are scoped to the rate-limiting table, ' + 'provider data table, and compact configuration table.',Also applies to: 177-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/stacks/api_lambda_stack/provider_management.py` at line 107, The existing AwsSolutions-IAM5 suppression reason text is outdated after adding a read grant to compact_configuration_table via compact_configuration_table.grant_read_data(handler); update the suppression comment(s) associated with AwsSolutions-IAM5 (the IAM suppression entries near the grant lines and the similar block around the other grant at the later location) to explicitly mention the new scoped read access to the compact configuration table and why it is required, ensuring the reason reflects the exact resource scope and the handler permission (grant_read_data) for audit clarity.backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py (1)
121-138: Optional: reuse the single template variable object instead of declaring two identical ones
expected_template_variables_ohandexpected_template_variables_necontain identical field values in every test — the template content is the same regardless of recipient jurisdiction. Declaring two separate variables adds noise without improving clarity.♻️ Proposed refactor (applies to all four happy-path test methods)
- expected_template_variables_oh = InvestigationNotificationTemplateVariables( - provider_first_name='Björk', - provider_last_name='Guðmundsdóttir', - investigation_jurisdiction=DEFAULT_LICENSE_JURISDICTION, - license_type='cosmetologist', - provider_id=UUID(DEFAULT_PROVIDER_ID), - ) - expected_template_variables_ne = InvestigationNotificationTemplateVariables( + expected_template_variables = InvestigationNotificationTemplateVariables( provider_first_name='Björk', provider_last_name='Guðmundsdóttir', investigation_jurisdiction=DEFAULT_LICENSE_JURISDICTION, license_type='cosmetologist', provider_id=UUID(DEFAULT_PROVIDER_ID), ) expected_state_calls = [ - {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_LICENSE_JURISDICTION, 'template_variables': expected_template_variables_oh}, - {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_PRIVILEGE_JURISDICTION, 'template_variables': expected_template_variables_ne}, + {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_LICENSE_JURISDICTION, 'template_variables': expected_template_variables}, + {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_PRIVILEGE_JURISDICTION, 'template_variables': expected_template_variables}, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py` around lines 121 - 138, The test repeats identical InvestigationNotificationTemplateVariables instances (expected_template_variables_oh and expected_template_variables_ne); replace the two duplicates with a single shared instance (e.g., expected_template_variables) and update expected_state_calls to reference that single variable so expected_state_calls still contains the two jurisdiction-specific dicts but without duplicated object declarations; adjust in all four happy-path test methods where expected_template_variables_oh / expected_template_variables_ne are declared.backend/cosmetology-app/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
118-126: Optional: convert list to set for O(1) membership check
live_jurisdictionsis alist[str]; theinoperator performs a linear scan. While jurisdiction counts are small enough that this is inconsequential today, converting to a set is a one-liner that makes the intent explicit.♻️ Proposed refactor
- live_jurisdictions = self.get_live_compact_jurisdictions(compact) - is_live = jurisdiction in live_jurisdictions + is_live = jurisdiction in set(self.get_live_compact_jurisdictions(compact))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/compact_configuration_client.py` around lines 118 - 126, The membership check in the method that calls self.get_live_compact_jurisdictions(compact) uses a list (live_jurisdictions) and should be converted to a set for O(1) membership tests; modify the code around live_jurisdictions and is_live (or update get_live_compact_jurisdictions to return a set) so you do is_live = jurisdiction in set(live_jurisdictions) (or return type set from get_live_compact_jurisdictions) and keep the same logging call that references compact, jurisdiction and is_live.backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
203-205: Add non-empty guards before per-item status assertions.These loops can pass even when
resultis empty, which makes the tests less effective at catching regressions in privilege generation.Proposed test hardening
@@ with self._patch_config_for_privilege_generation(resolution_date=date(2025, 1, 1)): pur = ProviderUserRecords(records) result = pur.generate_privileges_for_provider() + self.assertGreater(len(result), 0) for p in result: self.assertEqual(p['status'], 'inactive') @@ with self._patch_config_for_privilege_generation(resolution_date=date(2025, 1, 1)): pur = ProviderUserRecords(records) result = pur.generate_privileges_for_provider() + self.assertGreater(len(result), 0) for p in result: self.assertEqual(p['status'], 'active') @@ with self._patch_config_for_privilege_generation(resolution_date=date(2025, 1, 1)): pur = ProviderUserRecords(records) result = pur.generate_privileges_for_provider() + self.assertGreater(len(result), 0) for p in result: if p.get('jurisdiction') == 'al': self.assertEqual(p['status'], 'inactive')Also applies to: 223-224, 249-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py` around lines 203 - 205, The tests iterate over "result" and assert per-item status but don't check that "result" is non-empty, so add a non-empty guard before each loop (e.g., assertTrue(len(result) > 0) or assertGreater(len(result), 0)) to ensure the test fails when no privileges are generated; update the occurrences around the loops referencing "result" (including the blocks at the shown loop and the other occurrences mentioned at lines 223-224 and 249-253) to include this precondition before performing per-item assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cosmetology-app/lambdas/python/common/cc_common/config.py`:
- Around line 67-82: The docstring and type hint for the cached_property
live_compact_jurisdictions are inaccurate: get_live_compact_jurisdictions
returns a list of jurisdiction codes (str), not dicts, and the return type
dict[str, list] is underspecified; update the docstring to say "list of active
member jurisdiction strings (postal abbreviations)" and change the type
annotation to dict[str, list[str]] for the live_compact_jurisdictions property,
referencing the get_live_compact_jurisdictions call to ensure consistency.
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py`:
- Around line 497-510: The code currently filters licenses by compactEligibility
before selecting the latest, causing an older eligible license to be chosen
instead of the most recently issued one; change the logic in the block that
builds latest_compact_eligible_licenses (and the subsequent selection of
latest_issued_license into latest_issued_licenses_for_each_type) so you first
determine the most recently issued license for the type by sorting the full
licenses list by dateOfIssuance (descending) and picking index 0, then check
that license's compactEligibility (CompactEligibilityStatus) to decide whether
to append it to latest_issued_licenses_for_each_type; update references to
latest_compact_eligible_licenses, latest_issued_license, licenses, and
CompactEligibilityStatus.ELIGIBLE accordingly.
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py`:
- Around line 250-273: The success log currently uses len(live_jurisdictions)
which includes the excluded home jurisdiction and inflates the metric; update
the code to compute the actual number of published events (either by
incrementing a counter each time publish_privilege_encumbrance_event is invoked
inside the loop or by computing len([j for j in live_jurisdictions if j !=
license_jurisdiction])) and use that counter/filtered length in the logger call
(references: live_jurisdictions, license_jurisdiction,
publish_privilege_encumbrance_event, EventBatchWriter, logger).
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 656-666: The test currently leaves the default license adverse
action enabled (include_license_adverse_action=True) in the initial setup so the
provider can remain encumbered even without the second privilege adverse action;
update the test setup call that set include_license_adverse_action to False
(disable the default license encumbrance) so the only encumbrance comes from the
explicit second adverse action added with
put_default_adverse_action_record_in_provider_table, then run assertions to
verify the “other privilege encumbrances exist” behavior; keep references to
put_default_adverse_action_record_in_provider_table and
get_license_type_abbr_for_license_type to locate the relevant test lines.
---
Nitpick comments:
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/compact_configuration_client.py`:
- Around line 118-126: The membership check in the method that calls
self.get_live_compact_jurisdictions(compact) uses a list (live_jurisdictions)
and should be converted to a set for O(1) membership tests; modify the code
around live_jurisdictions and is_live (or update get_live_compact_jurisdictions
to return a set) so you do is_live = jurisdiction in set(live_jurisdictions) (or
return type set from get_live_compact_jurisdictions) and keep the same logging
call that references compact, jurisdiction and is_live.
In
`@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py`:
- Around line 203-205: The tests iterate over "result" and assert per-item
status but don't check that "result" is non-empty, so add a non-empty guard
before each loop (e.g., assertTrue(len(result) > 0) or
assertGreater(len(result), 0)) to ensure the test fails when no privileges are
generated; update the occurrences around the loops referencing "result"
(including the blocks at the shown loop and the other occurrences mentioned at
lines 223-224 and 249-253) to include this precondition before performing
per-item assertions.
In
`@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py`:
- Around line 121-138: The test repeats identical
InvestigationNotificationTemplateVariables instances
(expected_template_variables_oh and expected_template_variables_ne); replace the
two duplicates with a single shared instance (e.g., expected_template_variables)
and update expected_state_calls to reference that single variable so
expected_state_calls still contains the two jurisdiction-specific dicts but
without duplicated object declarations; adjust in all four happy-path test
methods where expected_template_variables_oh / expected_template_variables_ne
are declared.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py`:
- Around line 78-79: Update the stale inline comment that says “test privilege
record” to accurately describe the returned object as `license_record`; locate
the comment near the return statement that returns `test_event, license_record`
(referencing the variables `test_event` and `license_record`) and change the
wording to something like “return both the test event and the license record” so
the comment matches the actual returned value.
- Around line 55-57: Replace hard-coded 'ne' with the shared
DEFAULT_PRIVILEGE_JURISDICTION constant in the test setup and other affected
test lines: update the setUp() call that uses
set_live_compact_jurisdictions_for_test({'cosm': ['ne']}) to use
DEFAULT_PRIVILEGE_JURISDICTION instead, and make the same substitution at the
other occurrence around the investigation tests (the block referencing 'ne' at
the end of the file). This ensures tests use the shared
DEFAULT_PRIVILEGE_JURISDICTION symbol rather than a literal string.
In `@backend/cosmetology-app/stacks/api_lambda_stack/provider_management.py`:
- Line 107: The existing AwsSolutions-IAM5 suppression reason text is outdated
after adding a read grant to compact_configuration_table via
compact_configuration_table.grant_read_data(handler); update the suppression
comment(s) associated with AwsSolutions-IAM5 (the IAM suppression entries near
the grant lines and the similar block around the other grant at the later
location) to explicitly mention the new scoped read access to the compact
configuration table and why it is required, ensuring the reason reflects the
exact resource scope and the handler permission (grant_read_data) for audit
clarity.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
backend/cosmetology-app/lambdas/python/common/cc_common/config.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/compact_configuration_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/privilege/api.pybackend/cosmetology-app/lambdas/python/common/tests/function/test_data_client.pybackend/cosmetology-app/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/cosmetology-app/lambdas/python/common/tests/resources/dynamo/privilege-update.jsonbackend/cosmetology-app/lambdas/python/common/tests/resources/dynamo/privilege.jsonbackend/cosmetology-app/lambdas/python/common/tests/unit/test_config.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.pybackend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.pybackend/cosmetology-app/lambdas/python/data-events/handlers/license_deactivation_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/__init__.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_license_deactivation_events.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/investigation.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/__init__.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/cosmetology-app/stacks/api_lambda_stack/provider_management.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/event_listener_stack/__init__.pybackend/cosmetology-app/stacks/notification_stack.pybackend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/search_persistent_stack/__init__.pybackend/cosmetology-app/stacks/search_persistent_stack/populate_provider_documents_handler.pybackend/cosmetology-app/stacks/search_persistent_stack/provider_update_ingest_handler.pybackend/cosmetology-app/tests/app/test_event_listener.pybackend/cosmetology-app/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json
💤 Files with no reviewable changes (11)
- backend/cosmetology-app/lambdas/python/common/tests/resources/dynamo/privilege.json
- backend/cosmetology-app/lambdas/python/data-events/handlers/license_deactivation_events.py
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/privilege/api.py
- backend/cosmetology-app/lambdas/python/data-events/tests/function/test_license_deactivation_events.py
- backend/cosmetology-app/lambdas/python/common/tests/resources/api/provider-detail-response.json
- backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py
- backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py
- backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py
- backend/cosmetology-app/tests/app/test_event_listener.py
- backend/cosmetology-app/lambdas/python/common/tests/resources/dynamo/privilege-update.json
- backend/cosmetology-app/stacks/search_api_stack/v1_api/api_model.py
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py
Outdated
Show resolved
Hide resolved
backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
...etology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
657-661:⚠️ Potential issue | 🟠 MajorIsolate this test from license encumbrances to validate the intended scenario
Line 658 uses
_setup_privilege_with_adverse_action()with the defaultinclude_license_adverse_action=True, so provider encumbered status can remain true even if the added second privilege adverse action is incorrect. This weakens the “other privilege encumbrances exist” assertion.✅ Suggested fix
- context, adverse_action = self._setup_privilege_with_adverse_action() + context, adverse_action = self._setup_privilege_with_adverse_action( + include_license_adverse_action=False, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py` around lines 657 - 661, The test calls _setup_privilege_with_adverse_action() with its default include_license_adverse_action=True which may leave a license adverse action active and falsely keep the provider encumbered; change the setup call to pass include_license_adverse_action=False (or otherwise disable license adverse actions) so the first privilege's adverse action is strictly the non-license type being tested, then explicitly add the second non-license adverse action via test_data_generator.put_default_adverse_action_record_in_provider_table(...) to validate the “other privilege encumbrances exist” assertion; update any expectations/assertions accordingly to reflect the isolated non-license scenario.
🧹 Nitpick comments (1)
backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py (1)
121-137: Deduplicate the identical template-variable pairsIn every happy-path test,
expected_template_variables_ohandexpected_template_variables_necarry exactly the same field values. Defining two named variables implies they differ, which can confuse a future reader. Since the template variables represent the investigation details (not the notification recipient), a single variable reused for bothexpected_state_callsentries is clearer.♻️ Example for
test_license_investigation_listener_processes_event_with_registered_provider(same pattern applies to the other three happy-path tests)- expected_template_variables_oh = InvestigationNotificationTemplateVariables( + expected_template_variables = InvestigationNotificationTemplateVariables( provider_first_name='Björk', provider_last_name='Guðmundsdóttir', investigation_jurisdiction=DEFAULT_LICENSE_JURISDICTION, license_type='cosmetologist', provider_id=UUID(DEFAULT_PROVIDER_ID), ) - expected_template_variables_ne = InvestigationNotificationTemplateVariables( - provider_first_name='Björk', - provider_last_name='Guðmundsdóttir', - investigation_jurisdiction=DEFAULT_LICENSE_JURISDICTION, - license_type='cosmetologist', - provider_id=UUID(DEFAULT_PROVIDER_ID), - ) expected_state_calls = [ - {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_LICENSE_JURISDICTION, 'template_variables': expected_template_variables_oh}, - {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_PRIVILEGE_JURISDICTION, 'template_variables': expected_template_variables_ne}, + {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_LICENSE_JURISDICTION, 'template_variables': expected_template_variables}, + {'compact': DEFAULT_COMPACT, 'jurisdiction': DEFAULT_PRIVILEGE_JURISDICTION, 'template_variables': expected_template_variables}, ]Also applies to: 176-192, 227-243, 280-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py` around lines 121 - 137, The two variables expected_template_variables_oh and expected_template_variables_ne in the test (instances of InvestigationNotificationTemplateVariables) are identical; replace them with a single shared variable (e.g., expected_template_variables) and reuse it in the expected_state_calls list entries so both calls reference the same template_variables object, updating all four happy-path tests (including test_license_investigation_listener_processes_event_with_registered_provider) where the duplicate pairs appear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 657-661: The test calls _setup_privilege_with_adverse_action()
with its default include_license_adverse_action=True which may leave a license
adverse action active and falsely keep the provider encumbered; change the setup
call to pass include_license_adverse_action=False (or otherwise disable license
adverse actions) so the first privilege's adverse action is strictly the
non-license type being tested, then explicitly add the second non-license
adverse action via
test_data_generator.put_default_adverse_action_record_in_provider_table(...) to
validate the “other privilege encumbrances exist” assertion; update any
expectations/assertions accordingly to reflect the isolated non-license
scenario.
---
Nitpick comments:
In
`@backend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.py`:
- Around line 121-137: The two variables expected_template_variables_oh and
expected_template_variables_ne in the test (instances of
InvestigationNotificationTemplateVariables) are identical; replace them with a
single shared variable (e.g., expected_template_variables) and reuse it in the
expected_state_calls list entries so both calls reference the same
template_variables object, updating all four happy-path tests (including
test_license_investigation_listener_processes_event_with_registered_provider)
where the duplicate pairs appear.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/__init__.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/privilege/record.pybackend/cosmetology-app/lambdas/python/common/common_test/test_constants.pybackend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
💤 Files with no reviewable changes (3)
- backend/cosmetology-app/lambdas/python/common/common_test/test_constants.py
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/design/README.md`:
- Around line 346-349: Update the README to point to the actual routine that
implements renewal-first selection: replace or augment the reference to
ProviderRecordUtility.find_best_license with
ProviderUserRecords.generate_privileges_for_provider, and clarify that the
renewal-first (fall back to issuance date) selection logic is implemented in
ProviderUserRecords.generate_privileges_for_provider rather than
ProviderRecordUtility.find_best_license so readers can find the runtime
selection code.
- Around line 241-242: The internal anchor link "#privileges" in the sentence
describing stored record types is broken because the corresponding heading was
renamed; locate the heading introduced around the previous Line 330 (the current
privileges-related heading text) and update the fragment in the sentence to
match the exact new heading slug (or rename the heading back to "Privileges") so
the internal link resolves; ensure the anchor text in the sentence and the
heading text are identical after the change.
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py`:
- Around line 507-510: The loop that appends most_recent_license currently only
checks compactEligibility; extend it to also skip/licenses that are
inactive/expired or still encumbered. In the block around most_recent_license
(variables: most_recent_license, compactEligibility), add checks that
most_recent_license.status equals LicenseStatus.ACTIVE (or equivalent
active-state enum) and that most_recent_license.encumbrance (or is_encumbered
flag) does not indicate an unlifted/active encumbrance (e.g.,
EncumbranceStatus.UNLIFTED); if either check fails, continue and do not append.
Apply the same additional checks to the similar logic at the later block
handling selected/home license (the 534–540 area).
In
`@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py`:
- Around line 255-274: The test currently only iterates "for p in result" so it
passes vacuously if result is empty; update
test_status_inactive_when_license_expired (and the similar tests around it) to
first assert the returned value shape before the loop: assert that result is a
non-empty list (e.g., assertIsInstance(result, list) and
assertGreater(len(result), 0) or assertEqual(len(result), expected_count)), then
assert expected keys/fields exist on an example item and finally check
p['status'] == 'inactive'; reference
ProviderUserRecords.generate_privileges_for_provider and the local variable
result when adding these assertions.
- Line 216: The test assertion's failure message doesn't match the expected
value: update the message in the assertion
self.assertEqual(p['licenseJurisdiction'], 'al', 'Home should be OH (most
recently issued when no renewal)') to reflect the expected lowercase 'al' (e.g.,
'Home should be AL (most recently issued when no renewal)') so test failures are
accurate; locate this line in test_provider_record_util.py (the assertion
referencing p['licenseJurisdiction']) and replace the message string
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/cosmetology-app/docs/design/practitioner-home-state-license-assignment.pdfis excluded by!**/*.pdf
📒 Files selected for processing (3)
backend/cosmetology-app/docs/design/README.mdbackend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py
Outdated
Show resolved
Hide resolved
backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py
Outdated
Show resolved
Hide resolved
backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/design/README.md (1)
244-252:⚠️ Potential issue | 🟡 MinorAdd missing image asset referenced on line 341.
The file
practitioner-license-assignment-flow.pngis referenced in the documentation on line 341 but does not exist in thebackend/cosmetology-app/docs/design/directory. The referenced PDF (practitioner-home-state-license-assignment.pdf) exists, but the PNG diagram is missing. Either add the PNG file or remove/update the reference if the asset will not be provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/docs/design/README.md` around lines 244 - 252, The README references an image asset practitioner-license-assignment-flow.png that is missing; either add the PNG into the backend/cosmetology-app/docs/design/ directory (matching the referenced filename) or update the README.md to remove or replace the reference (e.g., point to the existing practitioner-home-state-license-assignment.pdf or embed a supported alternative). Locate the reference in README.md and ensure the link/path and filename (practitioner-license-assignment-flow.png) are consistent with the actual asset you add or change.
🤖 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/design/README.md`:
- Around line 341-343: The README.md references a missing image
(practitioner-license-assignment-flow.png) causing a broken link; either add the
missing PNG asset into the docs/design assets next to README.md using the same
filename, or remove the image markdown line and keep the PDF link
(practitioner-home-state-license-assignment.pdf) or replace the image reference
with an inline link to the PDF; update the README.md entry that currently
contains the image markdown to reflect whichever fix you choose.
In
`@backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py`:
- Around line 53-58: The test setup is populating the wrong field name for
compact eligibility; update the license_overrides_list entry created by
_make_provider_records to use the compactEligibility key instead of
jurisdictionUploadedCompactEligibility and set its value to
CompactEligibilityStatus.INELIGIBLE so the runtime privilege generation reads
the intended field (adjust any other test fixtures using
jurisdictionUploadedCompactEligibility accordingly).
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py`:
- Around line 250-270: The loop that fans out privilege events over
live_jurisdictions should first verify the source license is runtime-eligible
and skip publishing if not; update the block around live_jurisdictions and the
analogous block at lines 324-343 to call the existing runtime-eligibility check
(or add one if missing) using the source license attributes (provider_id,
license_type_abbreviation, effective_date or whatever the codebase uses) and
only iterate/publish via EventBusClient.publish_privilege_encumbrance_event when
that check returns true, leaving the license_jurisdiction exclusion logic
intact.
---
Outside diff comments:
In `@backend/cosmetology-app/docs/design/README.md`:
- Around line 244-252: The README references an image asset
practitioner-license-assignment-flow.png that is missing; either add the PNG
into the backend/cosmetology-app/docs/design/ directory (matching the referenced
filename) or update the README.md to remove or replace the reference (e.g.,
point to the existing practitioner-home-state-license-assignment.pdf or embed a
supported alternative). Locate the reference in README.md and ensure the
link/path and filename (practitioner-license-assignment-flow.png) are consistent
with the actual asset you add or change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/cosmetology-app/docs/design/README.mdbackend/cosmetology-app/lambdas/python/common/cc_common/config.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.pybackend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py
backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py
Show resolved
Hide resolved
backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/data-events/handlers/investigation_events.py (1)
185-195:⚠️ Potential issue | 🔴 CriticalRemove stale
provider_recordskwarg from_send_additional_state_notificationscall sites.At Line 188, Line 256, Line 318, and Line 386,
provider_recordsis passed but the helper no longer takes it as a formal parameter; it is forwarded via**notification_kwargsintoInvestigationNotificationTemplateVariables, which can break notification construction at runtime.Proposed fix
_send_additional_state_notifications( config.email_service_client.send_license_investigation_state_notification_email, 'license investigation', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_license_investigation_closed_state_notification_email, 'license investigation closed', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_privilege_investigation_state_notification_email, 'privilege investigation', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_privilege_investigation_closed_state_notification_email, 'privilege investigation closed', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id,Also applies to: 253-263, 315-325, 383-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.py` around lines 185 - 195, Call sites of _send_additional_state_notifications are still passing the removed provider_records kwarg (e.g. the call using config.email_service_client.send_license_investigation_state_notification_email and the other calls at the same pattern), which will break because the helper no longer accepts provider_records; remove the provider_records=provider_records argument from all invocations of _send_additional_state_notifications (including the occurrences around the license investigation call and the other similar calls at the indicated blocks) so that provider_records is only passed via **notification_kwargs / InvestigationNotificationTemplateVariables as intended.backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py (1)
397-411:⚠️ Potential issue | 🔴 CriticalRemove stale
provider_recordskwarg from additional-state notification calls.At Line 400, Line 521, Line 592, and Line 688,
provider_recordsis passed to_send_additional_state_notifications, but that function now forwards unknown kwargs intoEncumbranceNotificationTemplateVariables. This can raise a runtime constructor error.Proposed fix
_send_additional_state_notifications( config.email_service_client.send_privilege_encumbrance_state_notification_email, 'privilege encumbrance', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_privilege_encumbrance_lifting_state_notification_email, 'privilege encumbrance lifting', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_license_encumbrance_state_notification_email, 'license encumbrance', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id, @@ _send_additional_state_notifications( config.email_service_client.send_license_encumbrance_lifting_state_notification_email, 'license encumbrance lifting', - provider_records=provider_records, provider_record=provider_record, provider_id=provider_id,Also applies to: 518-532, 589-603, 685-699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py` around lines 397 - 411, The call sites of _send_additional_state_notifications are passing a stale provider_records keyword which is not accepted by the downstream EncumbranceNotificationTemplateVariables and causes constructor errors; remove the provider_records=provider_records argument from every _send_additional_state_notifications invocation (e.g., the call that uses config.email_service_client.send_privilege_encumbrance_state_notification_email and the other analogous calls in this file) so only the valid kwargs (provider_record, provider_id, excluded_jurisdiction/jurisdiction, compact, event_type, event_time, tracker, encumbered_jurisdiction, license_type, effective_date, etc.) are forwarded. Ensure no other call sites in this module still pass provider_records.
♻️ Duplicate comments (3)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
656-667:⚠️ Potential issue | 🟠 MajorDisable the default license encumbrance in this privilege-only scenario.
At Line 656, the helper default leaves a license adverse action active, so this test can still pass even if the second privilege encumbrance logic regresses.
Suggested patch
- context, adverse_action = self._setup_privilege_with_adverse_action() + context, adverse_action = self._setup_privilege_with_adverse_action( + include_license_adverse_action=False, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py` around lines 656 - 667, The test helper _setup_privilege_with_adverse_action currently leaves a default license adverse action active which masks regressions; update the test setup so the default license encumbrance is disabled before adding the second privilege adverse action—either remove or mark the license adverse action inactive via test_data_generator (e.g., call the helper to delete the default license record or override its fields/status) so only privilege-based adverse actions determine encumbrance when using put_default_adverse_action_record_in_provider_table.backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py (1)
245-268:⚠️ Potential issue | 🟠 MajorGate privilege event fan-out on compact eligibility before publishing.
At Line 248 and Line 322, fan-out is based only on live jurisdictions. This still emits privilege encumbrance/lift events for licenses that are not compact-eligible, which conflicts with the runtime privilege-generation behavior in this PR.
Proposed fix
-from cc_common.data_model.schema.common import LicenseEncumberedStatusEnum +from cc_common.data_model.schema.common import CompactEligibilityStatus, LicenseEncumberedStatusEnum @@ def license_encumbrance_listener(message: dict): @@ - live_jurisdictions = config.live_compact_jurisdictions.get(compact, []) + provider_user_records = config.data_client.get_provider_user_records( + compact=compact, provider_id=provider_id, consistent_read=True + ) + source_license = provider_user_records.get_specific_license_record( + jurisdiction=license_jurisdiction, + license_abbreviation=license_type_abbreviation, + ) + if source_license is None: + raise CCInternalException('No license record found for the specified jurisdiction and license type') + if source_license.compactEligibility != CompactEligibilityStatus.ELIGIBLE: + logger.info('License is not compact-eligible. Skipping privilege encumbrance fan-out.') + return + + live_jurisdictions = config.live_compact_jurisdictions.get(compact, []) @@ def license_encumbrance_lifted_listener(message: dict): @@ - if license_record.encumberedStatus == LicenseEncumberedStatusEnum.ENCUMBERED: + if license_record.compactEligibility != CompactEligibilityStatus.ELIGIBLE: + logger.info('License is not compact-eligible. Not sending privilege encumbrance lift notifications.') + return + if license_record.encumberedStatus == LicenseEncumberedStatusEnum.ENCUMBERED: logger.info('License is still encumbered. Not sending privilege encumbrance lift notifications.') returnAlso applies to: 322-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py` around lines 245 - 268, Fan-out is currently done for all live compact jurisdictions regardless of whether the underlying license is compact-eligible; update the encumbrance and lift publishing blocks (the loop using EventBatchWriter and calls to event_bus_client.publish_privilege_encumbrance_event / publish_privilege_lift_event) to first verify the license is compact-eligible for this compact (use the existing compact eligibility check/utility or config value used elsewhere in this PR) and only iterate/publish for live_jurisdictions when that eligibility check passes; apply the same gating logic at both the encumbrance loop (where license_jurisdiction, compact, provider_id, license_type_abbreviation, effective_date are used) and the corresponding lift loop so events are not emitted for non-eligible licenses.backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py (1)
530-537: Same unguardedprivileges[0]index access after close.Line 532 has the same issue as Line 144: a direct index into
privilegeswithout confirming the list is non-empty first. The suggestion from the earlier test applies here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py` around lines 530 - 537, The test directly indexes provider_data['privileges'][0] without confirming the list is non-empty, causing a potential IndexError; update the test (in this test_investigation case) to first assert that 'privileges' exists and has at least one element (e.g., assertTrue or assertGreater(len(...), 0) on provider_data['privileges']) before assigning privilege = provider_data['privileges'][0], then perform the existing comparison of privilege['investigations'] to the expected empty list. Ensure you reference provider_data and privilege variables when adding the guard.
🧹 Nitpick comments (6)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
118-120: Use the context license-type abbreviation in the SK prefix assertion.At Line 119, the hardcoded
coscan drift from test context and cause brittle assertions if defaults change.Suggested patch
- sk_prefix = f'{context["compact"]}#PROVIDER#privilege/{context["jurisdiction"]}/cos#ADVERSE_ACTION' + sk_prefix = ( + f'{context["compact"]}#PROVIDER#privilege/{context["jurisdiction"]}/' + f'{context["licenseTypeAbbreviation"]}#ADVERSE_ACTION' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py` around lines 118 - 120, The SK prefix is hardcoded with 'cos' which makes the test brittle; update the sk_prefix construction in the test (the variable named sk_prefix) to use the license-type abbreviation from the test context instead of the literal 'cos' (e.g. interpolate context["licenseTypeAbbreviation"] into the string, keeping the existing context["compact"], context["jurisdiction"] and other parts intact) so the assertion follows the test context.backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
499-505: Move_effective_datehelper outside the loop.
_effective_dateis unconditionally redefined on every loop iteration. Since it has no dependency on loop variables, hoisting it above theforblock eliminates the redundant re-definition and makes the intent clearer.♻️ Proposed refactor
+ def _effective_date(lic: LicenseData) -> date: + return lic.dateOfRenewal if lic.dateOfRenewal is not None else lic.dateOfIssuance + most_recent_licenses_for_each_type: list[LicenseData] = [] for _lt, licenses in by_type.items(): - # Sort all licenses of this type: effective_date = dateOfRenewal or dateOfIssuance - def _effective_date(lic: LicenseData): - return lic.dateOfRenewal if lic.dateOfRenewal is not None else lic.dateOfRenewal - sorted_licenses = sorted(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py` around lines 499 - 505, The helper _effective_date is being defined inside the loop/iteration context and gets redefined on each iteration; move its definition out (declare it once above where sorted_licenses is computed) so it’s defined just once and then reuse it in the sorted call (keep the same logic using lic.dateOfRenewal if present else lic.dateOfIssuance), ensuring sorted_licenses = sorted(licenses, key=lambda x: (_effective_date(x), x.dateOfIssuance), reverse=True) still references the hoisted _effective_date.
609-638: Dead initial assignment ofprivileges.
privileges = []on Line 609 is immediately overwritten byprivileges = self.generate_privileges_for_provider()on Line 638 without ever being read. The initialisation is dead code.♻️ Proposed fix
provider = self.get_provider_record().to_dict() licenses = [] - privileges = [] # Build licenses dict with investigations and adverseActions for license_record in self._license_records: ... licenses.append(license_dict) - # Build privileges at runtime from eligible licenses (one privilege per license type per compact jurisdiction) - privileges = self.generate_privileges_for_provider() + # Build privileges at runtime from eligible licenses (one privilege per license type per compact jurisdiction) + privileges = self.generate_privileges_for_provider()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py` around lines 609 - 638, Remove the dead initial assignment privileges = [] at the top of the method: the variable is immediately replaced by privileges = self.generate_privileges_for_provider() later, so delete the first assignment to avoid unused code; verify that no other code between the two assignments depends on privileges (the method that builds licenses uses a local variable licenses and not privileges) and run tests to confirm no behavioral changes in the method that includes generate_privileges_for_provider.backend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.py (1)
882-905:open_investigationsin the privilege branch is computed but never consumed.
is_last_open_investigation_against_license(Lines 903–905) only fires wheninvestigation_against == InvestigationAgainstEnum.LICENSE, so theopen_investigationslist built in theelsebranch (Lines 883–887) is always discarded. The redundantinv.closeDate is Noneguard in the filter condition (Line 886) is also unnecessary becauseget_investigation_records_for_privilegealready excludes closed records by default (include_closed=False).♻️ Proposed simplification
else: - # Privilege: no stored privilege record; find investigation by jurisdiction/license type only. - open_investigations = provider_records.get_investigation_records_for_privilege( - jurisdiction, - license_type_abbreviation, - filter_condition=lambda inv: inv.closeDate is None and inv.investigationId != investigation_id, - ) - investigation = next( + # Privilege: no stored privilege record; find investigation by jurisdiction/license type only. + investigation = next( ( inv for inv in provider_records.get_investigation_records_for_privilege(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.py` around lines 882 - 905, The open_investigations list built by provider_records.get_investigation_records_for_privilege (assigned to open_investigations) is never used in the privilege branch; remove that unused call and the redundant inv.closeDate filter, and only call get_investigation_records_for_privilege when you actually need to evaluate is_last_open_investigation_against_license (i.e., compute len(open_investigations) on demand inside the is_last_open_investigation_against_license check when investigation_against == InvestigationAgainstEnum.LICENSE), so eliminate the redundant construction of open_investigations and the unnecessary inv.closeDate guard to simplify the privilege branch.backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py (1)
143-164: Add an assertion before indexingprivileges[0]to get a clear error on empty list.Line 144 directly indexes
provider_data['privileges'][0]without first asserting the list is non-empty. If no privileges are generated (e.g., live jurisdiction config changes or home jurisdiction matches the live jurisdiction), the test fails with an unhelpfulIndexErrorinstead of a descriptive assertion failure.♻️ Proposed guard
+ self.assertEqual(1, len(provider_data['privileges']), 'Expected exactly one runtime-generated privilege') privilege = provider_data['privileges'][0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py` around lines 143 - 164, Before indexing provider_data['privileges'][0] in the test, add an explicit assertion that the privileges list is present and non-empty (e.g., assertTrue/provider_data.get('privileges') or assertGreater(len(provider_data['privileges']), 0)) so the test fails with a clear message when no privileges are generated; place this guard immediately before the line that assigns privilege = provider_data['privileges'][0] and include a descriptive failure message like "Expected at least one privilege but none were generated".backend/cosmetology-app/lambdas/python/common/tests/function/test_data_client.py (1)
174-180:provider_records.provider_recordsrelies on an internal attribute — prefer the public accessor.Accessing
provider_records.provider_recordscouples the test to the internal storage ofProviderUserRecords. If the class later stops retaining the raw iterable, the assertion breaks silently. Prefer using the publicget_license_records()/get_provider_record()accessors already used later in the test, or compare the sum of counts from those accessors.♻️ Proposed alternative assertion
- # We expect 1 provider record + 30 license records = 31 total - self.assertEqual(31, len(provider_records.provider_records)) + # We expect 1 provider record + 30 license records = 31 total + self.assertEqual(1, len(provider_records._provider_records)) + self.assertEqual(30, len(provider_records.get_license_records()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/common/tests/function/test_data_client.py` around lines 174 - 180, The test is accessing the internal attribute provider_records.provider_records; change assertions to use the public accessors on ProviderUserRecords instead: use get_provider_record() and get_license_records() (called on the provider_records instance) to compute total count (1 + len(get_license_records())) and to build the set of record types (e.g., include 'provider' if get_provider_record() is present and 'license' if get_license_records() returns items); update the two assertions to compare against these values rather than provider_records.provider_records.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 497-504: The test currently reuses adverse_action_overrides when
inserting a license adverse-action record which leaks privilege-specific
scenario state; update the call to
put_default_adverse_action_record_in_provider_table for the license case to use
a separate overrides map (e.g., license_adverse_action_overrides) or build a
fresh dict that only contains license-specific keys instead of spreading
adverse_action_overrides; specifically change the value_overrides passed to the
call where actionAgainst is 'license' so it does not include the shared
adverse_action_overrides variable and only applies license-scoped overrides.
---
Outside diff comments:
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py`:
- Around line 397-411: The call sites of _send_additional_state_notifications
are passing a stale provider_records keyword which is not accepted by the
downstream EncumbranceNotificationTemplateVariables and causes constructor
errors; remove the provider_records=provider_records argument from every
_send_additional_state_notifications invocation (e.g., the call that uses
config.email_service_client.send_privilege_encumbrance_state_notification_email
and the other analogous calls in this file) so only the valid kwargs
(provider_record, provider_id, excluded_jurisdiction/jurisdiction, compact,
event_type, event_time, tracker, encumbered_jurisdiction, license_type,
effective_date, etc.) are forwarded. Ensure no other call sites in this module
still pass provider_records.
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.py`:
- Around line 185-195: Call sites of _send_additional_state_notifications are
still passing the removed provider_records kwarg (e.g. the call using
config.email_service_client.send_license_investigation_state_notification_email
and the other calls at the same pattern), which will break because the helper no
longer accepts provider_records; remove the provider_records=provider_records
argument from all invocations of _send_additional_state_notifications (including
the occurrences around the license investigation call and the other similar
calls at the indicated blocks) so that provider_records is only passed via
**notification_kwargs / InvestigationNotificationTemplateVariables as intended.
---
Duplicate comments:
In
`@backend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.py`:
- Around line 245-268: Fan-out is currently done for all live compact
jurisdictions regardless of whether the underlying license is compact-eligible;
update the encumbrance and lift publishing blocks (the loop using
EventBatchWriter and calls to
event_bus_client.publish_privilege_encumbrance_event /
publish_privilege_lift_event) to first verify the license is compact-eligible
for this compact (use the existing compact eligibility check/utility or config
value used elsewhere in this PR) and only iterate/publish for live_jurisdictions
when that eligibility check passes; apply the same gating logic at both the
encumbrance loop (where license_jurisdiction, compact, provider_id,
license_type_abbreviation, effective_date are used) and the corresponding lift
loop so events are not emitted for non-eligible licenses.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 656-667: The test helper _setup_privilege_with_adverse_action
currently leaves a default license adverse action active which masks
regressions; update the test setup so the default license encumbrance is
disabled before adding the second privilege adverse action—either remove or mark
the license adverse action inactive via test_data_generator (e.g., call the
helper to delete the default license record or override its fields/status) so
only privilege-based adverse actions determine encumbrance when using
put_default_adverse_action_record_in_provider_table.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py`:
- Around line 530-537: The test directly indexes provider_data['privileges'][0]
without confirming the list is non-empty, causing a potential IndexError; update
the test (in this test_investigation case) to first assert that 'privileges'
exists and has at least one element (e.g., assertTrue or assertGreater(len(...),
0) on provider_data['privileges']) before assigning privilege =
provider_data['privileges'][0], then perform the existing comparison of
privilege['investigations'] to the expected empty list. Ensure you reference
provider_data and privilege variables when adding the guard.
---
Nitpick comments:
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 882-905: The open_investigations list built by
provider_records.get_investigation_records_for_privilege (assigned to
open_investigations) is never used in the privilege branch; remove that unused
call and the redundant inv.closeDate filter, and only call
get_investigation_records_for_privilege when you actually need to evaluate
is_last_open_investigation_against_license (i.e., compute
len(open_investigations) on demand inside the
is_last_open_investigation_against_license check when investigation_against ==
InvestigationAgainstEnum.LICENSE), so eliminate the redundant construction of
open_investigations and the unnecessary inv.closeDate guard to simplify the
privilege branch.
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py`:
- Around line 499-505: The helper _effective_date is being defined inside the
loop/iteration context and gets redefined on each iteration; move its definition
out (declare it once above where sorted_licenses is computed) so it’s defined
just once and then reuse it in the sorted call (keep the same logic using
lic.dateOfRenewal if present else lic.dateOfIssuance), ensuring sorted_licenses
= sorted(licenses, key=lambda x: (_effective_date(x), x.dateOfIssuance),
reverse=True) still references the hoisted _effective_date.
- Around line 609-638: Remove the dead initial assignment privileges = [] at the
top of the method: the variable is immediately replaced by privileges =
self.generate_privileges_for_provider() later, so delete the first assignment to
avoid unused code; verify that no other code between the two assignments depends
on privileges (the method that builds licenses uses a local variable licenses
and not privileges) and run tests to confirm no behavioral changes in the method
that includes generate_privileges_for_provider.
In
`@backend/cosmetology-app/lambdas/python/common/tests/function/test_data_client.py`:
- Around line 174-180: The test is accessing the internal attribute
provider_records.provider_records; change assertions to use the public accessors
on ProviderUserRecords instead: use get_provider_record() and
get_license_records() (called on the provider_records instance) to compute total
count (1 + len(get_license_records())) and to build the set of record types
(e.g., include 'provider' if get_provider_record() is present and 'license' if
get_license_records() returns items); update the two assertions to compare
against these values rather than provider_records.provider_records.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py`:
- Around line 118-120: The SK prefix is hardcoded with 'cos' which makes the
test brittle; update the sk_prefix construction in the test (the variable named
sk_prefix) to use the license-type abbreviation from the test context instead of
the literal 'cos' (e.g. interpolate context["licenseTypeAbbreviation"] into the
string, keeping the existing context["compact"], context["jurisdiction"] and
other parts intact) so the assertion follows the test context.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.py`:
- Around line 143-164: Before indexing provider_data['privileges'][0] in the
test, add an explicit assertion that the privileges list is present and
non-empty (e.g., assertTrue/provider_data.get('privileges') or
assertGreater(len(provider_data['privileges']), 0)) so the test fails with a
clear message when no privileges are generated; place this guard immediately
before the line that assigns privilege = provider_data['privileges'][0] and
include a descriptive failure message like "Expected at least one privilege but
none were generated".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/data_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.pybackend/cosmetology-app/lambdas/python/common/tests/function/test_data_client.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_base_record.pybackend/cosmetology-app/lambdas/python/data-events/handlers/encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/handlers/investigation_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/__init__.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_encumbrance_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_investigation.pybackend/cosmetology-app/stacks/search_persistent_stack/provider_update_ingest_handler.py
💤 Files with no reviewable changes (2)
- backend/cosmetology-app/lambdas/python/common/common_test/test_data_generator.py
- backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_data_model/test_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/cosmetology-app/stacks/search_persistent_stack/provider_update_ingest_handler.py
...etology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cosmetology will not track privileges in its DB, as we do with JCC. Privileges will be generated at request time based on which jurisdictions are currently live in the system. If a practitioner's license is not eligible, no privilege objects will be generated.
We will continue tracking adverse actions and investigations in the DB. These will be used to generate the full practitioner detail document used by the frontend so state admins can manage encumbrances/investigations.
This removes references to persistent privilege records and adds logic for generating the privileges at request time.
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 #1282
Summary by CodeRabbit
New Features
Changes
Tests & Docs