Send email notification to former home state when provider home state changes#1548
Send email notification to former home state when provider home state changes#1548landonshumway-ia wants to merge 13 commits intocsg-org:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements home jurisdiction change notifications end-to-end: ingest detects same-license-type jurisdiction changes and emits a provider.homeStateChange EventBridge event; stack wiring delivers events to an SQS listener that uses EmailServiceClient to invoke the NodeJS email Lambda, which resolves former-jurisdiction recipients and sends the notification. Notification-only handlers now use top-level provider record lookups. ChangesHome Jurisdiction Change Notification
Staff User Permissions Enhancement
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider Data<br/>Ingest
participant EventBus as EventBridge
participant SQS as SQS Queue
participant Listener as Home State<br/>Change Listener
participant Client as Email Service<br/>Client
participant Lambda as Email<br/>Notification Lambda
participant SES as SES Service
Provider->>Provider: Detect home jurisdiction<br/>change (best license)
Provider->>EventBus: Publish provider.homeStateChange<br/>event (former jurisdiction)
EventBus->>SQS: Route event to queue
Listener->>SQS: Poll message
Listener->>Listener: Load event detail &<br/>extract fields
Listener->>Client: send_provider_home_state_change_email<br/>(former jurisdiction, variables)
Client->>Lambda: Invoke with<br/>homeJurisdictionChangeNotification
Lambda->>Lambda: Resolve former jurisdiction<br/>operations team recipients
Lambda->>Lambda: Build email template<br/>with provider details
Lambda->>SES: SendEmail to operations team
SES-->>Lambda: Email sent
Lambda-->>Client: Success response
Client-->>Listener: Notification complete
Listener->>Listener: Log completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py (1)
705-722: 💤 Low valueConsider replacing the hardcoded
providerIdwith the capturedprovider_idvariable.The assertion at line 712 hardcodes
'89a6377e-c3a5-40e5-bca5-317ec854c570'. If the SSN fixture changes, this test breaks for an unrelated reason.provider_idis already captured on line 671 and can be used directly.♻️ Proposed fix
- self.assertEqual( - { - 'Detail': json.dumps( - { - 'compact': 'cosm', - 'jurisdiction': 'ky', - 'eventTime': '2024-11-08T23:59:59+00:00', - 'providerId': '89a6377e-c3a5-40e5-bca5-317ec854c570', - 'licenseType': 'cosmetologist', - 'formerHomeJurisdiction': 'oh', - } - ), - 'DetailType': 'provider.homeStateChange', - 'EventBusName': 'license-data-events', - 'Source': 'org.compactconnect.provider-data', - }, - home_change_entry, - ) + self.assertEqual('provider.homeStateChange', home_change_entry['DetailType']) + self.assertEqual('license-data-events', home_change_entry['EventBusName']) + self.assertEqual('org.compactconnect.provider-data', home_change_entry['Source']) + detail = json.loads(home_change_entry['Detail']) + self.assertEqual('cosm', detail['compact']) + self.assertEqual('ky', detail['jurisdiction']) + self.assertEqual('2024-11-08T23:59:59+00:00', detail['eventTime']) + self.assertEqual(provider_id, detail['providerId']) + self.assertEqual('cosmetologist', detail['licenseType']) + self.assertEqual('oh', detail['formerHomeJurisdiction'])🤖 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 test assertion hardcodes the provider UUID; replace the literal '89a6377e-c3a5-40e5-bca5-317ec854c570' with the captured variable provider_id so the assertion compares home_change_entry to the expected dict using provider_id (the variable assigned earlier on line ~671) instead of a fixed string; update the expected payload construction in the assertion that references 'providerId' to use provider_id to avoid brittle tests.backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
93-127: 💤 Low valueLGTM!
The
license_typefilter is cleanly applied after the existinghome_jurisdictionfilter, and the two-stage narrowing is correct. The empty-list →CCInternalExceptionpath is handled identically to the pre-existing behavior.One nit: the docstring has two items numbered
2.(lines 104–105). Should be 1/2/3.🤖 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/data_model/provider_record_util.py` around lines 93 - 127, Update the docstring in find_best_license to fix the numbered list (replace the duplicated "2." with the correct sequence 1/2/3) so the steps read "1. If home jurisdiction..., 2. If license type..., 3. Return the single license..." — edit the docstring block at the start of the method in provider_record_util.py (the find_best_license method) to correct the numbering only.backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py (1)
137-137: ⚡ Quick winPrefer direct key access
['licenseType']over.get('licenseType')for fail-fast behavior.Both
find_best_licensecalls passlicense_type=posted_license_record.get('licenseType'). IflicenseTypewere somehow absent from the record,.get()silently returnsNone, disabling the type filter and causingfind_best_licenseto return the overall best license instead of the best license for the posted type. This could trigger spurious or missed home-state-change notifications across license types.Since
licenseTypeis required by the ingest schema, using['licenseType']would fail fast and make the invariant explicit.♻️ Proposed fix
- current_best_license_for_posted_license_type = ProviderRecordUtility.find_best_license( - license_records=license_records, license_type=posted_license_record.get('licenseType') - ) + current_best_license_for_posted_license_type = ProviderRecordUtility.find_best_license( + license_records=license_records, license_type=posted_license_record['licenseType'] + )- best_license_after_upload_for_posted_license_type = ProviderRecordUtility.find_best_license( - license_records=licenses_flattened, license_type=posted_license_record.get('licenseType') - ) + best_license_after_upload_for_posted_license_type = ProviderRecordUtility.find_best_license( + license_records=licenses_flattened, license_type=posted_license_record['licenseType'] + )Also applies to: 222-224
🤖 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/handlers/ingest.py` at line 137, The call sites that pass license_type=posted_license_record.get('licenseType') to find_best_license should use direct indexing posted_license_record['licenseType'] to fail fast when the required key is missing; update both occurrences where find_best_license(..., license_records=license_records, license_type=posted_license_record.get('licenseType')) appears so the function receives the explicit value and will raise immediately if 'licenseType' is absent, preserving the ingest schema invariant and avoiding silent None behavior.
🤖 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/lambdas/nodejs/lib/email/email-notification-service.ts`:
- Around line 18-22: The SPECIFIC branch in getJurisdictionRecipients currently
treats an empty array as present because it checks truthiness; update
getJurisdictionRecipients to explicitly require a non-empty array (e.g., check
Array.isArray(specificEmails) && specificEmails.length > 0) and throw the
existing error if specificEmails is missing or empty; ensure callers like
sendHomeJurisdictionChangeStateNotificationEmail (and any other callers)
continue to rely on the function returning a valid non-empty recipient array or
receiving an exception.
In `@backend/cosmetology-app/stacks/notification_stack.py`:
- Around line 246-258: The construct_id_prefix passed in
_add_provider_home_state_change_notification_listener is not PascalCase; change
the string 'ProviderhomeJurisdictionChangeNotificationListener' to
'ProviderHomeJurisdictionChangeNotificationListener' so CDK logical IDs follow
the established convention; update only the construct_id_prefix argument in the
call inside _add_provider_home_state_change_notification_listener to the
corrected PascalCase value to avoid creating an inconsistent CloudFormation
logical ID.
---
Nitpick comments:
In
`@backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py`:
- Around line 93-127: Update the docstring in find_best_license to fix the
numbered list (replace the duplicated "2." with the correct sequence 1/2/3) so
the steps read "1. If home jurisdiction..., 2. If license type..., 3. Return the
single license..." — edit the docstring block at the start of the method in
provider_record_util.py (the find_best_license method) to correct the numbering
only.
In `@backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py`:
- Line 137: The call sites that pass
license_type=posted_license_record.get('licenseType') to find_best_license
should use direct indexing posted_license_record['licenseType'] to fail fast
when the required key is missing; update both occurrences where
find_best_license(..., license_records=license_records,
license_type=posted_license_record.get('licenseType')) appears so the function
receives the explicit value and will raise immediately if 'licenseType' is
absent, preserving the ingest schema invariant and avoiding silent None
behavior.
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py`:
- Around line 705-722: The test assertion hardcodes the provider UUID; replace
the literal '89a6377e-c3a5-40e5-bca5-317ec854c570' with the captured variable
provider_id so the assertion compares home_change_entry to the expected dict
using provider_id (the variable assigned earlier on line ~671) instead of a
fixed string; update the expected payload construction in the assertion that
references 'providerId' to use provider_id to avoid brittle tests.
🪄 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: 357a6f73-4930-45df-b693-ea20a2c35c37
📒 Files selected for processing (21)
backend/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/tests/email-notification-service.test.tsbackend/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/email_service_client.pybackend/cosmetology-app/lambdas/python/common/cc_common/event_bus_client.pybackend/cosmetology-app/lambdas/python/common/tests/unit/test_email_service_client.pybackend/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/tests/function/test_home_state_change_events.pybackend/cosmetology-app/lambdas/python/data-events/tests/function/test_investigation_events.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.pybackend/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/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/notification_stack.py
💤 Files with no reviewable changes (1)
- backend/cosmetology-app/lambdas/nodejs/lib/email/base-email-service.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tests/smoke/license_upload_smoke_tests.py`:
- Around line 112-138: The query in _wait_for_home_state_change_event only
inspects the first DynamoDB page and can miss matching provider.homeStateChange
events; update the logic around data_events_table.query and
response.get('Items') to handle pagination by looping on
LastEvaluatedKey/ExclusiveStartKey (accumulating or streaming each page) until
the matching item with providerId == provider_id is found or pages exhausted,
preserving ConsistentRead and poll backoff behavior; apply the same pagination
fix to the other helper referenced (the similar query at the lines noted) so
both functions check all pages instead of just the first.
- Around line 141-151: _query_license_ingest_events_for_jurisdiction currently
returns all license.ingest events in a jurisdiction/time window and the finally
block deletes them all; change the query to only return records for the current
test provider before passing them to _cleanup_test_generated_records by adding a
provider parameter (e.g., provider: str) to
_query_license_ingest_events_for_jurisdiction and include the provider in the
KeyConditionExpression or a FilterExpression (e.g., ensure the sk includes
'TYPE#license.ingest#PROVIDER#{provider}#TIME#' or use FilterExpression with
ExpressionAttributeValues[':provider'] and begins_with/contains on sk) so only
that provider's ingest events are returned; update any callers (the finally
cleanup code and places referenced around lines 255-271) to pass the fixture
provider and ensure only those records are forwarded to
_cleanup_test_generated_records.
In `@backend/cosmetology-app/tests/smoke/smoke_common.py`:
- Around line 367-369: The poll loop undercounts attempts because max_attempts =
max_wait_time // poll_interval_seconds omits the initial t=0 attempt and skips
polling when max_wait_time < poll_interval_seconds; update the calculation to
include the initial attempt by using max_attempts = max(1, max_wait_time //
poll_interval_seconds + 1) (using the existing variables max_wait_time and
poll_interval_seconds and assigned to max_attempts) so the loop in the function
that uses max_attempts performs the initial check at t=0 and covers the full
max_wait_time window.
🪄 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: a12aca22-c97e-46e0-bf49-f4f046b73e5f
📒 Files selected for processing (4)
backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.tsbackend/cosmetology-app/tests/smoke/license_upload_smoke_tests.pybackend/cosmetology-app/tests/smoke/smoke_common.pybackend/multi-account/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/cosmetology-app/lambdas/nodejs/lib/email/email-notification-service.ts
| max_wait_time: int = 300, | ||
| staff_user_email: str | None = None, | ||
| poll_interval_seconds: int = 30, |
There was a problem hiding this comment.
This poller stops one interval early.
max_attempts = max_wait_time // poll_interval_seconds undercounts because the first lookup happens at t=0. With the new 720/60 settings, the last retry is around 660s, not 720s, and values smaller than the poll interval skip polling altogether.
Suggested fix
+ if poll_interval_seconds <= 0:
+ raise ValueError('poll_interval_seconds must be greater than 0')
+
start_time = time.time()
attempts = 0
- max_attempts = max_wait_time // poll_interval_seconds
+ max_attempts = max(1, (max_wait_time // poll_interval_seconds) + 1)Also applies to: 389-405
🤖 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 367 - 369,
The poll loop undercounts attempts because max_attempts = max_wait_time //
poll_interval_seconds omits the initial t=0 attempt and skips polling when
max_wait_time < poll_interval_seconds; update the calculation to include the
initial attempt by using max_attempts = max(1, max_wait_time //
poll_interval_seconds + 1) (using the existing variables max_wait_time and
poll_interval_seconds and assigned to max_attempts) so the loop in the function
that uses max_attempts performs the initial check at t=0 and covers the full
max_wait_time window.
There was a problem hiding this comment.
I have increased the wait time to account for this.
There was a problem hiding this comment.
@landonshumway-ia Understood — using a larger wait time is a reasonable workaround for the practical impact. Just note that the underlying calculation still stops one interval short (e.g., with 720/60, the loop caps at 12 attempts covering ~660s, not 720s), so if the wait time is ever tightened in the future, the off-by-one could resurface. Feel free to leave a comment in the code noting this behaviour if you'd like to make it easier to catch later. Happy to leave it as-is otherwise!
🧠 Learnings used
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1277
File: backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py:78-78
Timestamp: 2026-02-09T17:37:05.334Z
Learning: Enforce a unique constraint on privilege records so that (provider_id, jurisdiction_id, license_type_id) combinations are unique. A provider can have multiple privileges within the same jurisdiction only if each privilege has a distinct license_type. Implement this at the data/model layer (e.g., unique constraint in the database schema) and validate in tests to prevent duplicates across (jurisdiction, license_type) per provider while allowing multiple licenses per jurisdiction.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1299
File: backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py:477-486
Timestamp: 2026-02-26T19:59:47.770Z
Learning: In provider_record_util.py (and related Python modules under backend/cosmetology-app), rely on and enforce the existing normalization pattern where jurisdiction values are stored in lowercase. Verify that config.live_compact_jurisdictions.get(compact, []) yields lowercase values and ensure any comparisons or storage also use lowercase (or enforce lowercase normalization at the data surface) to remain consistent with the app-wide normalization strategy.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1314
File: backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py:477-480
Timestamp: 2026-03-25T13:18:59.552Z
Learning: In the cosmetology backend (backend/cosmetology-app), treat the `jurisdiction` value on `InvestigationData`/investigation records as the jurisdiction of the *target entity being investigated* (the privilege/license), not the jurisdiction conducting the investigation. This should be consistent with the `generate_sk` key structure, where `{investigation_against}/{jurisdiction}/{license_type_abbr}...` encodes the investigated privilege/license jurisdiction. When reviewing code that builds/queries investigation records (e.g., `get_investigation_records_for_privilege(privilege_jurisdiction, license_type_abbr)`), ensure parameters and key/predicate logic map to the investigated privilege/license jurisdiction. Do not apply this field interpretation to the frontend/main compact-connect cross-jurisdiction pattern; avoid conflating the two meanings.
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1326
File: backend/cosmetology-app/tests/smoke/license_upload_smoke_tests.py:223-224
Timestamp: 2026-04-13T13:59:40.148Z
Learning: In cosmetology smoke test scripts (e.g., files under `backend/cosmetology-app/tests/smoke/`), it is acceptable for `except SmokeTestFailureException:` handlers to log the failure but not re-raise and not call `sys.exit(1)`—these tests are intended for local developer runs where logs are reviewed directly. Do not flag this as a bug solely for not returning a non-zero exit code, as long as the exception is handled explicitly and the error is clearly logged.
|
|
||
| 1. If home jurisdiction is selected, only consider licenses from that jurisdiction | ||
| 2. If license type is specified, only consider licenses of that type. | ||
| 2. Return the single license with the latest (renewal date or issuance date) |
There was a problem hiding this comment.
We've got 1) 2) 2) here
| :param license_type: License type filter | ||
| :return: The best license record | ||
| """ | ||
| # If the provider's home jurisdiction was selected, we only consider licenses from that jurisdiction |
There was a problem hiding this comment.
This isnt true anymore right?
| ) | ||
| if license_records_in_jurisdiction: | ||
| license_records = license_records_in_jurisdiction | ||
| license_records = license_records_in_jurisdiction |
There was a problem hiding this comment.
So do they not select a home jurisdiction when registering?
If they do then you could be comparing the new license vs the not-most-recent license because they selected a home jurisdiction in a state that does not have their most recent license.
If they dont and the home state is always based on the same logic then this would work but is a kindof unintuitive and slightly more brittle shortcut.
I guess I would expect the find best license function under these parameters to just look at all their licenses and get the most recent one, not compare the one that caused the flow with the licenses of that type in their home state.
| license_records = license_records_in_jurisdiction | ||
| license_records = license_records_in_jurisdiction | ||
|
|
||
| if license_type is not None: |
There was a problem hiding this comment.
Separately, Im not 100% understanding the interplay of license types here.
If my most recent license is type A in MA (12/1/2026) but I have type B in MA as well (10/1/2026) and then LA uploads license Type B (11/1/2026), wouldnt we switch home state to LA with this?
Maybe Im just misunderstanding the description of the feature but I thought we would want to stay home state MA in that case?
| self.assertEqual('Audrey', ky_license['givenName']) | ||
|
|
||
| self.assertEqual('ky', provider_data['licenseJurisdiction']) | ||
| self.assertEqual('Audrey', provider_data['givenName']) |
There was a problem hiding this comment.
Should we have a negative test too? It uploads but doesnt trigger change?
| self.assertEqual('Audrey', ky_license['givenName']) | ||
|
|
||
| self.assertEqual('ky', provider_data['licenseJurisdiction']) | ||
| self.assertEqual('Audrey', provider_data['givenName']) |
There was a problem hiding this comment.
Somewhere we should have tests asserting the behavior with MA and LA and different license types that I mentioned.
As it is I dont know what the intended behavior is and so we should codify / concrete that through tests
ChiefStief
left a comment
There was a problem hiding this comment.
Looks like a good start. I've got some tests I think should get added + some questions about intended functionality. Depending on the answers we may have some bugs.
For the Cosmetology compact, the home state for a practitioner is determined by which license was issued or renewed most recently. If another state uploads or renews a license record for that same practitioner with a more recent date, that state becomes the new home state for that practitioner. In that case, the system needs to send a notification to the former home state to let them know that the practitioner's home state has changed.
This adds a new event notification listener that sends an email when triggered. It also updates the license ingest
logic to compare the previous best license with the uploaded license.
Requirements List
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 #1543
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation