[Social Work] - Deactivate Staff accounts after 60 days of inactivity - #1829
Open
landonshumway-ia wants to merge 21 commits into
Open
[Social Work] - Deactivate Staff accounts after 60 days of inactivity#1829landonshumway-ia wants to merge 21 commits into
landonshumway-ia wants to merge 21 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds staff-user inactivity tracking and lifecycle processing. The change records logins, identifies inactive users, sends reminder emails, deactivates users after the inactivity period, tracks workflow state, and provisions daily scheduled infrastructure. ChangesStaff User Inactivity
Estimated code review effort: 5 (Critical) | ~90 minutes Mergeability Score: ⚪ Minimal · up to This change adds automated staff-account inactivity notifications and deactivation behavior; no actionable merge-blocking risk remains based on the supplied evidence, so it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant EventBridge
participant StaffUserInactivityHandler
participant CompactStaffUserDirectory
participant StaffUserInactivityTracker
participant EmailServiceClient
participant UserClient
EventBridge->>StaffUserInactivityHandler: Invoke scheduled inactivity event
StaffUserInactivityHandler->>CompactStaffUserDirectory: Select inactive users and administrators
StaffUserInactivityHandler->>StaffUserInactivityTracker: Check and record workflow steps
StaffUserInactivityHandler->>EmailServiceClient: Send user and administrator emails
StaffUserInactivityHandler->>UserClient: Deactivate user on day-of run
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
backend/social-work-app/lambdas/python/staff-users/staff_user_inactivity_tracker.py (1)
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared clock for the TTL.
The rest of this feature reads time from
config.current_standard_datetime, and the function tests patch that property. This line readstime.time()directly, so the TTL is not controlled by the test clock. The TTL is not asserted today, so behavior is correct. Aligning the time source keeps one clock for the module.♻️ Proposed change
- 'ttl': int(time.time()) + int(timedelta(days=self._TTL_DAYS).total_seconds()), + 'ttl': int((config.current_standard_datetime + timedelta(days=self._TTL_DAYS)).timestamp()),Remove the now-unused
import timeif no other reference remains.🤖 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/social-work-app/lambdas/python/staff-users/staff_user_inactivity_tracker.py` at line 101, Update the TTL calculation in the inactivity-tracking flow to derive the current timestamp from config.current_standard_datetime instead of time.time(), preserving the existing _TTL_DAYS offset. Remove the time import if it is no longer referenced elsewhere in the module.backend/social-work-app/lambdas/nodejs/email-notification-service/lambda.ts (1)
352-358: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTighten the validation of
inactivityPeriodDaysanddeactivationDate.Two points in this validation block:
!event.templateVariables?.inactivityPeriodDaysrejects the numeric value0. Use an explicitundefined/nullcheck for the number field.- The block checks presence of
deactivationDatebut not its format.formatIsoDateAsSlashFormatinlib/email/email-notification-service.tscallsNumberon each segment. A non-YYYY-MM-DDstring rendersNaN/NaN/undefinedin the subject and body instead of failing.The current Python producer always sends
60and an ISO date, so neither case is reachable today. Both guards protect the email content against a future producer change.♻️ Proposed validation
case 'staffUserInactivityNotification': if (!event.templateVariables?.staffUserFirstName || !event.templateVariables?.staffUserLastName || !event.templateVariables?.staffUserEmail || !event.templateVariables?.deactivationDate - || !event.templateVariables?.inactivityPeriodDays) { + || event.templateVariables?.inactivityPeriodDays === undefined + || event.templateVariables?.inactivityPeriodDays === null) { throw new Error('Missing required template variables for staffUserInactivityNotification template.'); } + if (!/^\d{4}-\d{2}-\d{2}$/.test(event.templateVariables.deactivationDate)) { + throw new Error('Invalid deactivationDate for staffUserInactivityNotification template. Expected YYYY-MM-DD.'); + }🤖 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/social-work-app/lambdas/nodejs/email-notification-service/lambda.ts` around lines 352 - 358, Update the validation block for the staffUserInactivityNotification template to check inactivityPeriodDays explicitly for null/undefined so numeric 0 is accepted, and validate deactivationDate as a valid YYYY-MM-DD date before proceeding. Preserve the existing required-field checks and throw the same missing-template-variables error when either value is invalid.backend/social-work-app/lambdas/nodejs/tests/email-notification-service.test.ts (1)
1187-1201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the formatted date in the success test.
The test asserts the recipient only. The new
formatIsoDateAsSlashFormathelper converts2026-09-14to09/14/2026. Add an assertion on the rendered subject so a regression in the date format fails this test.♻️ Proposed additional assertion
expect(mockSESClient).toHaveReceivedCommandWith(SendEmailCommand, { Destination: { ToAddresses: ['jane@example.com'] - } + }, + Content: expect.objectContaining({ + Simple: expect.objectContaining({ + Subject: { Data: 'CompactConnect account for Jane Smith will be deactivated on 09/14/2026' } + }) + }) });Confirm the
Contentshape against the existing SES assertions in this file before applying.🤖 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/social-work-app/lambdas/nodejs/tests/email-notification-service.test.ts` around lines 1187 - 1201, Update the successful staff inactivity notification test to assert the SES SendEmailCommand subject includes the formatted date “09/14/2026,” using the existing Content shape and SES assertion patterns in the file. Keep the recipient assertion unchanged and target the rendered subject produced by formatIsoDateAsSlashFormat.backend/social-work-app/lambdas/python/staff-users/handlers/staff_user_inactivity.py (1)
250-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
already_donemixes email skips and deactivation skips.
Metrics.record_email_outcomeincrementsalready_donefor a skipped email. This line increments the same counter for a skipped deactivation. A run that reportsalreadyDone: 5does not say which steps were skipped, which makes reconciliation of a partial run harder.Add a separate counter for skipped deactivations.
♻️ Proposed change
`@dataclass` class Metrics: ... deactivated: int = 0 deactivations_failed: int = 0 + deactivations_already_done: int = 0def as_dict(self) -> dict[str, int]: return { ... 'deactivated': self.deactivated, 'deactivationsFailed': self.deactivations_failed, + 'deactivationsAlreadyDone': self.deactivations_already_done, }if tracker.was_already_done(InactivityStep.DEACTIVATION): - metrics.already_done += 1 + metrics.deactivations_already_done += 1 return
test_already_notified_users_are_not_emailed_twiceassertsalreadyDone > 0on a reminder run, so it stays valid.🤖 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/social-work-app/lambdas/python/staff-users/handlers/staff_user_inactivity.py` around lines 250 - 252, Replace the deactivation skip increment in the handler’s InactivityStep.DEACTIVATION branch with a dedicated skipped-deactivation metric, leaving Metrics.record_email_outcome and its already_done behavior unchanged. Define or reuse the corresponding Metrics field and ensure the reported metrics expose this separate counter for reconciliation.backend/social-work-app/lambdas/python/staff-users/tests/function/test_handlers/test_staff_user_inactivity.py (1)
218-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a partial email failure.
test_an_email_failure_is_counted_and_does_not_abort_the_runfails every send. The handler tracksUSER_EMAILandADMIN_EMAILas separate steps so that one failure retries only the failed half. No handler-level test proves that wiring.Add a case where the admin send fails and the user send succeeds. Assert that a second run re-sends only to the admins.
💚 Proposed test
def test_a_failed_admin_send_is_retried_without_re_emailing_the_user(self): _, user_email = self._seed_user(days_since_login=51, jurisdictions={'oh': {WRITE}}) _, admin_email = self._seed_user(days_since_login=1, jurisdictions={'oh': {ADMIN}}) def fail_admin_only(**kwargs): if kwargs['recipient_emails'] == [admin_email]: raise RuntimeError('SES is down') with patch('cc_common.config._Config.email_service_client') as mock_email_client: mock_email_client.send_staff_user_inactivity_notification_email.side_effect = fail_admin_only self._run(days_before=10) with patch('cc_common.config._Config.email_service_client') as mock_email_client: result = self._run(days_before=10) self.assertEqual([[admin_email]], self._recipient_sets(mock_email_client)) self.assertEqual(0, result['metrics']['userEmailsSent'])🤖 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/social-work-app/lambdas/python/staff-users/tests/function/test_handlers/test_staff_user_inactivity.py` around lines 218 - 229, Add a handler-level test alongside test_an_email_failure_is_counted_and_does_not_abort_the_run that seeds a user and admin, makes only the admin recipient send fail, then runs the handler again with successful sending. Assert the second run invokes the email client only for the admin recipient and reports zero user emails sent, proving USER_EMAIL succeeds once while ADMIN_EMAIL is retried.
🤖 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/social-work-app/lambdas/python/common/cc_common/data_model/user_client.py`:
- Around line 168-176: Update the login-recording DynamoDB update in the
surrounding user-client method to conditionally succeed only while the user
record remains active and its lifecycle version is unchanged; prevent the stale
pre-token invocation from setting status back to StaffUserStatus.ACTIVE after
deactivate_user begins. Reuse the existing lifecycle/version field and handle
the conditional failure as a rejected login write, then add a concurrency test
covering login loading user_data before deactivation and attempting the update
afterward.
In `@backend/social-work-app/lambdas/python/staff-user-pre-token/main.py`:
- Around line 42-51: Make recording the login event mandatory in the pre-token
handler: update the flow around record_user_login so token issuance does not
continue when persistence fails. Remove the swallowed exception behavior and
propagate the failure, or otherwise ensure a durable retry/verification state
prevents inactivity deactivation until the login is recorded.
In
`@backend/social-work-app/lambdas/python/staff-users/handlers/staff_user_inactivity.py`:
- Around line 184-210: Update the comment above the user and admin sends to
describe only that they are separate sends with independent retry behavior;
remove the claim that one admin cannot see another admin’s address. Do not
change the existing recipient batching or tracking logic in the USER_EMAIL and
ADMIN_EMAIL flows.
In `@backend/social-work-app/lambdas/python/staff-users/staff_user_directory.py`:
- Around line 27-32: Update the administrator-bucket population in the
staff-user directory initialization to add users only when their status equals
StaffUserStatus.ACTIVE.value, covering both _compact_admins and
_jurisdiction_admins while preserving self._users for all users. Add regression
coverage for inactive compact and jurisdiction administrators to ensure
resolve_admin_recipients excludes them.
---
Nitpick comments:
In `@backend/social-work-app/lambdas/nodejs/email-notification-service/lambda.ts`:
- Around line 352-358: Update the validation block for the
staffUserInactivityNotification template to check inactivityPeriodDays
explicitly for null/undefined so numeric 0 is accepted, and validate
deactivationDate as a valid YYYY-MM-DD date before proceeding. Preserve the
existing required-field checks and throw the same missing-template-variables
error when either value is invalid.
In
`@backend/social-work-app/lambdas/nodejs/tests/email-notification-service.test.ts`:
- Around line 1187-1201: Update the successful staff inactivity notification
test to assert the SES SendEmailCommand subject includes the formatted date
“09/14/2026,” using the existing Content shape and SES assertion patterns in the
file. Keep the recipient assertion unchanged and target the rendered subject
produced by formatIsoDateAsSlashFormat.
In
`@backend/social-work-app/lambdas/python/staff-users/handlers/staff_user_inactivity.py`:
- Around line 250-252: Replace the deactivation skip increment in the handler’s
InactivityStep.DEACTIVATION branch with a dedicated skipped-deactivation metric,
leaving Metrics.record_email_outcome and its already_done behavior unchanged.
Define or reuse the corresponding Metrics field and ensure the reported metrics
expose this separate counter for reconciliation.
In
`@backend/social-work-app/lambdas/python/staff-users/staff_user_inactivity_tracker.py`:
- Line 101: Update the TTL calculation in the inactivity-tracking flow to derive
the current timestamp from config.current_standard_datetime instead of
time.time(), preserving the existing _TTL_DAYS offset. Remove the time import if
it is no longer referenced elsewhere in the module.
In
`@backend/social-work-app/lambdas/python/staff-users/tests/function/test_handlers/test_staff_user_inactivity.py`:
- Around line 218-229: Add a handler-level test alongside
test_an_email_failure_is_counted_and_does_not_abort_the_run that seeds a user
and admin, makes only the admin recipient send fail, then runs the handler again
with successful sending. Assert the second run invokes the email client only for
the admin recipient and reports zero user emails sent, proving USER_EMAIL
succeeds once while ADMIN_EMAIL is retried.
🪄 Autofix
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 Plus
Run ID: 447b5744-7d34-41b7-af06-904a250da9de
📒 Files selected for processing (33)
backend/social-work-app/lambdas/nodejs/email-notification-service/lambda.tsbackend/social-work-app/lambdas/nodejs/lib/email/email-notification-service.tsbackend/social-work-app/lambdas/nodejs/tests/email-notification-service.test.tsbackend/social-work-app/lambdas/nodejs/tests/lib/email/email-notification-service.test.tsbackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/user/__init__.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/user/api.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/schema/user/record.pybackend/social-work-app/lambdas/python/common/cc_common/data_model/user_client.pybackend/social-work-app/lambdas/python/common/cc_common/email_service_client.pybackend/social-work-app/lambdas/python/common/common_test/test_constants.pybackend/social-work-app/lambdas/python/common/common_test/test_data_generator.pybackend/social-work-app/lambdas/python/common/tests/function/test_data_model/test_user_client.pybackend/social-work-app/lambdas/python/common/tests/unit/test_email_service_client.pybackend/social-work-app/lambdas/python/staff-user-pre-token/main.pybackend/social-work-app/lambdas/python/staff-user-pre-token/tests/test_main.pybackend/social-work-app/lambdas/python/staff-users/handlers/staff_user_inactivity.pybackend/social-work-app/lambdas/python/staff-users/staff_user_directory.pybackend/social-work-app/lambdas/python/staff-users/staff_user_inactivity_tracker.pybackend/social-work-app/lambdas/python/staff-users/tests/__init__.pybackend/social-work-app/lambdas/python/staff-users/tests/function/__init__.pybackend/social-work-app/lambdas/python/staff-users/tests/function/test_handlers/test_get_users.pybackend/social-work-app/lambdas/python/staff-users/tests/function/test_handlers/test_staff_user_inactivity.pybackend/social-work-app/lambdas/python/staff-users/tests/function/test_staff_user_inactivity_tracker.pybackend/social-work-app/lambdas/python/staff-users/tests/unit/staff_user_test_data.pybackend/social-work-app/lambdas/python/staff-users/tests/unit/test_data_model/test_schema/test_user.pybackend/social-work-app/lambdas/python/staff-users/tests/unit/test_resolve_admin_recipients.pybackend/social-work-app/lambdas/python/staff-users/tests/unit/test_staff_user_directory.pybackend/social-work-app/pipeline/backend_stage.pybackend/social-work-app/stacks/api_lambda_stack/staff_users.pybackend/social-work-app/stacks/staff_user_inactivity_stack.pybackend/social-work-app/tests/app/base.pybackend/social-work-app/tests/app/test_api/test_staff_users_api.pybackend/social-work-app/tests/app/test_staff_user_inactivity_stack.py
landonshumway-ia
force-pushed
the
feat/sw-staff-user-inactive-notifications
branch
from
August 14, 2026 18:25
fdcae0a to
46b7fe5
Compare
Collaborator
Author
|
@jlkravitz This is now ready for your review. Thanks |
landonshumway-ia
force-pushed
the
feat/sw-staff-user-inactive-notifications
branch
from
August 18, 2026 17:23
96a07a0 to
4ab179d
Compare
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.
The commission has requested that staff accounts automatically expire after they have not logged in after 60 days. Before reaching that point we will send out a notification 10 days prior, 3 days prior, and the last day before their account is deactivated. This adds a new CDK stack for a scheduled job that runs daily to list all of the staff users for the compact, checks the last time they logged in, and sends out notifications to users that match the specific period of inactivity notifying them that they need to log back in. The notifications are also sent to the associated state admins for the jurisdiction if the user only has permissions in a specific jurisdiction, or the compact admins if the user to be deactivated is the only state admin.
In order to support this, we needed to start tracking a 'lastLoginAt' timestamp field on the staff user DynamoDB record. This field is now updated in our token generation hook every time the user successfully logs in and generates an access token.
This also updates the re-invite user endpoint to re-enable the Cognito user so that an admin staff user can re-invite a user that has been deactivated.
The original implementation for staff user permission and attribute updates did not update the timestamp of the date. Although the UI does not rely on this change, it helps devops track when the latest changes have occurred on a record. This also updates the staff user related logic to update that field when changes are made.
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 #1668
Summary by CodeRabbit
New Features
Bug Fixes