Privilege Expiry Email Notifications#1277
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a privilege-expiration reminder feature: OpenSearch-backed Python handler with continuation and idempotency, Node.js email template and handler path, a CDK stack with EventBridge rules and VPC endpoint, CI pip upgrades and dependency bumps, plus extensive tests and smoke/load tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant EB as EventBridge
participant Lambda as ExpirationReminder<br/>Lambda
participant OS as OpenSearch
participant DDB as EventState<br/>(DynamoDB)
participant EmailLambda as Email<br/>Notification Lambda
participant SES as SES/Email
EB->>Lambda: Invoke (daysBefore, compact, targetDate)
Lambda->>Lambda: Validate inputs and compute targetDate
rect rgba(200,150,100,0.5)
loop fetch pages
Lambda->>OS: Query active privileges expiring on targetDate (search_after)
OS->>Lambda: Return provider batch + search_after
end
end
rect rgba(100,150,200,0.5)
loop each provider in batch
Lambda->>DDB: Check tracker (was_already_sent)
alt already sent
Lambda->>Lambda: Skip provider (metrics.already_sent++)
else first-time
Lambda->>Lambda: Build email template (active privileges)
Lambda->>EmailLambda: invoke sendPrivilegeExpirationReminderEmail(payload)
EmailLambda->>SES: Submit email
SES-->>EmailLambda: Delivery/response
Lambda->>DDB: Record SUCCESS with TTL
end
end
end
alt nearing timeout
Lambda->>Lambda: Emit continuation event (search_after) and self-invoke
end
Lambda->>Lambda: Return aggregated metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
landonshumway-ia
force-pushed
the
feat/expiry-notifications
branch
from
6c56e8d to
1388b2c
Compare
landonshumway-ia
force-pushed
the
feat/expiry-notifications
branch
from
a6791e6 to
1873e2b
Compare
This required adding a pre-load hook to the API response schemas to check for licenses/privileges that may have expired in OpenSearch since their last update in DynamoDB.
This will make background processes more robust to temp connection timeouts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt (1)
7-20:⚠️ Potential issue | 🔴 Criticalbotocore==1.42.42 does not exist on PyPI; latest available is 1.42.40.
The requirements-dev.txt pins botocore==1.42.42, but this version is not available on PyPI (latest as of Feb 5, 2026 is 1.42.40). boto3==1.42.42 and cryptography==46.0.4 are available, but the botocore pin will cause dev/test environment installs to fail. Update the pin to botocore==1.42.40 or verify with the source requirements-dev.in file to ensure the compiled output reflects available versions.
🤖 Fix all issues with AI agents
In @.github/workflows/check-compact-connect-ui-app.yml:
- Around line 113-114: The workflow step named "Install all Python dependencies"
currently runs the wrong directory ("backend/compact-connect;
bin/sync_deps.sh"); update the run command to use the correct path
"backend/compact-connect-ui-app; bin/sync_deps.sh" so the bin/sync_deps.sh
script is executed in the compact-connect-ui-app directory, matching the other
steps in this workflow.
In `@backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts`:
- Around line 389-422: The switch case for 'privilegeExpirationReminder'
declares lexical bindings (e.g., the const privileges) that leak across the
switch — wrap the entire case body in a block to limit scope: add an opening
brace immediately after case 'privilegeExpirationReminder': and a closing brace
just before the break so all declarations (privileges, p, etc.) are
block-scoped; keep the existing validation and the call to
this.emailService.sendPrivilegeExpirationReminderEmail intact (ensure the break
remains inside the block).
In
`@backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py`:
- Around line 102-105: The current error logging in the send-email block logs
full payload and response (payload, response) which may contain PII; modify the
error path in the method that checks response.get('FunctionError') so it logs
only sanitized metadata (e.g., notification_id, provider name, status/code) or a
redacted version of payload/response rather than the full objects, and keep
raising CCInternalException(error_message) with the same high-level message;
implement or call a helper (e.g., redact_email_payload or sanitize_response) to
strip fields like recipient addresses, names, and message bodies before passing
data to self._logger.error.
In
`@backend/compact-connect/lambdas/python/search/handlers/expiration_reminders.py`:
- Around line 35-79: The Metrics field matched_privileges / matchedPrivileges is
defined and serialized but never incremented; update the code that builds the
recipient/email list for each provider (where provider_doc /
PaginatedProviderResult are processed) to count matched privileges per provider
and add that count to the running Metrics.matched_privileges, ensure you
increment the existing metrics when aggregating across pages/continuations (use
the 'matchedPrivileges' key consistently with _initialize_metrics and
Metrics.as_dict), and include this increment when constructing or updating
Metrics instances so reported metrics reflect the actual matched-privilege
total.
- Around line 314-337: The email_privileges list is currently populated with all
active privileges; update the loop that iterates provider_doc['privileges'] (the
privilege processing block that calls
CompactConfigUtility.get_jurisdiction_name) to additionally filter each
privilege by the target expiration date (e.g., compare
privilege['dateOfExpiration'] to the target_date/target_expiration_date variable
used elsewhere in this handler) before appending to email_privileges; keep the
existing active-status check and jurisdiction-display validation, log/skip
non-matching or invalid entries, and only append privileges whose
dateOfExpiration matches the target expiration date.
In
`@backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py`:
- Around line 627-634: grant_search_providers only attaches an identity-based
policy via
principal.grant_principal.add_to_principal_policy(self._get_search_policy()) but
does not add the corresponding resource-based domain policy, causing OpenSearch
403s; fix by also adding the same policy statement to the domain resource policy
(e.g., call self.domain.add_to_resource_policy(self._get_search_policy()) or the
equivalent domain.add_access_policy API) inside grant_search_providers so
principals get both identity- and resource-based permissions (mirror what
_configure_access_policies / domain.grant_* do).
In `@backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py`:
- Around line 360-374: The code incorrectly assumes LoggingConfig.LogGroup is an
ARN and splits on 'log-group:'; instead use the value of
logging_config.get('LogGroup') directly for filter_log_events and defensively
strip any trailing ':*' suffix if present before assigning log_group_name (refer
to variables logging_config, log_group_arn, log_group_name and context around
lambda_name); remove the split('log-group:') logic and replace it with direct
assignment plus a simple suffix strip.
In `@backend/multi-account/README.md`:
- Around line 111-112: Update the README sentence that instructs users not to
update runtime code or delete critical resources to use tighter wording: replace
the phrase "outside of our CI/CD review" with "outside our CI/CD review and
deployment process" in the guidance about applying the inline policy to the
AWSPowerUserAccess permission set so the policy description reads clearly and
unambiguously.
In `@IMPLEMENTATION_PLAN.md`:
- Around line 128-217: The plan's file paths and TTL description are out of sync
with the code: update the document to reference the actual implementation
locations and TTL behavior (or change code if plan is authoritative).
Specifically, change references from lambdas/python/expiration-reminders/ to
lambdas/python/search/ and point the tracker reference to
cc_common.event_state_client. Also correct the TTL statement to reflect that
ExpirationReminderTracker (or EventStateTable) computes TTL based on write time
rather than "90 days after expiration" (or alternatively adjust the tracker to
compute TTL from expiration date); update mentions of
process_expiration_reminders, iterate_privileges_by_expiration_date,
send_privilege_expiration_reminder_email, and
ExpirationReminderTracker/EventStateTable so the plan and implementation use the
same paths and TTL semantics.
🧹 Nitpick comments (7)
backend/multi-account/README.md (1)
119-149: Clarify the policy name vs. statementSidto avoid operator confusion.The instructions now name the policy
DenyComputeBackupAndResourceModifications, but the JSON still usesSid: DenyComputeAndBackupUpdates. Consider aligning theSidto the new policy name (or explicitly note that the policy name and statementSidare different) to reduce copy/paste mistakes during manual setup.Also applies to: 212-214
backend/cosmetology-app/lambdas/python/common/tests/unit/test_sanitize_provider_data.py (1)
71-83: Consider reusing a shared frozen datetime constant.This avoids repeating the literal and centralizes the date if it needs to change later.
♻️ Proposed refactor
from datetime import datetime from unittest.mock import patch +FROZEN_DATETIME = datetime.fromisoformat('2025-04-01T12:00:00+00:00') + @@ -@patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat('2025-04-01T12:00:00+00:00')) +@patch('cc_common.config._Config.current_standard_datetime', FROZEN_DATETIME) def test_sanitized_provider_record_returned_if_caller_does_not_have_read_private_permissions_for_jurisdiction( self, ): @@ -@patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat('2025-04-01T12:00:00+00:00')) +@patch('cc_common.config._Config.current_standard_datetime', FROZEN_DATETIME) def test_sanitized_provider_record_returned_if_caller_does_not_have_any_read_private_permissions(self):Also applies to: 80-83
backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py (1)
77-89: Consider reusing a shared frozen datetime constant.This avoids repeating the literal and centralizes the date if it needs to change later.
♻️ Proposed refactor
from datetime import datetime from unittest.mock import patch +FROZEN_DATETIME = datetime.fromisoformat('2025-04-01T12:00:00+00:00') + @@ -@patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat('2025-04-01T12:00:00+00:00')) +@patch('cc_common.config._Config.current_standard_datetime', FROZEN_DATETIME) def test_sanitized_provider_record_returned_if_caller_does_not_have_read_private_permissions_for_jurisdiction( self, ): @@ -@patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat('2025-04-01T12:00:00+00:00')) +@patch('cc_common.config._Config.current_standard_datetime', FROZEN_DATETIME) def test_sanitized_provider_record_returned_if_caller_does_not_have_any_read_private_permissions(self):backend/compact-connect/stacks/vpc_stack/__init__.py (1)
27-33: Confirm the S3 endpoint is still justified before documenting it as default.Line 32 now lists S3 as a standard endpoint in this stack. Prior guidance for this file says not to include an S3 Gateway endpoint by default unless snapshots to S3 or Lambda runtime S3 access are being introduced. Please confirm the need; if it’s still not required, consider removing the S3 endpoint (or gating it) rather than codifying it in docs.
Based on learnings: In backend/compact-connect/stacks/vpc_stack/init.py, do not add an S3 Gateway VPC endpoint by default for the OpenSearch VPC stack. S3 access is not required at this time; add the endpoint only if OpenSearch snapshots to S3 or Lambda runtime S3 access are introduced.
backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py (1)
227-237: Remove the stale docstring param in_configure_access_policies.
compact_abbreviationsis no longer a parameter, so the docstring should reflect instance usage.✂️ Suggested docstring tweak
- :param compact_abbreviations: List of compact abbreviations for index access policiesbackend/compact-connect/stacks/expiration_reminder_stack/__init__.py (1)
111-138: Minor: Timezone comment may be misleading during Standard Time.The comment states "midnight UTC-4 (4:00 AM UTC)" but UTC-4 only applies during Eastern Daylight Time (EDT). During Eastern Standard Time (EST), 4:00 AM UTC corresponds to 11:00 PM EST (the previous day). If the intent is to run at a consistent local time, consider using a time that accounts for daylight saving transitions, or clarify that the schedule uses a fixed UTC time regardless of DST.
The per-compact rule design is good - it enables isolated failure handling and clear CloudWatch metrics per compact.
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py (1)
86-318: Consider a more linear, top-to-bottom smoke-test flow.The helper-heavy structure makes the execution path less “story-like” for manual runs. If feasible, consider consolidating or reordering helpers to keep the main flow readable in a single pass.
Based on learnings: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/search/handlers/expiration_reminders.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/search/handlers/expiration_reminders.py
Show resolved
Hide resolved
backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review (can wait until after the next prod deployment). Thanks |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/compact-connect/lambdas/python/cognito-backup/requirements.txt`:
- Around line 9-11: The cognito-backup requirements explicitly pins boto3,
botocore, and aws-lambda-powertools which duplicates the shared lambda layer;
remove those three entries from cognito-backup/requirements.in so the function
relies on the common requirements layer instead, leaving any other non-shared
dependencies intact and ensuring consistency with other Lambdas that rely on the
common requirements.in.
In
`@backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py`:
- Around line 381-383: The test is patching
cc_common.config._Config.current_standard_datetime using new_callable which sets
a callable instead of a datetime; change the patch to use new= with the actual
datetime value (e.g., compute datetime.now(tz=UTC).replace(microsecond=0) -
timedelta(days=randint(1,365))) so current_standard_datetime is a datetime
instance rather than a function; update the patch site where
current_standard_datetime is referenced to replace new_callable=... with new=...
and ensure the computed datetime expression is evaluated at patch time.
In `@backend/compact-connect/tests/app/test_expiration_reminder_stack.py`:
- Around line 127-128: The comment above the assertion for duration_alarm is
wrong: update the comment to state "14 minutes (840,000 milliseconds)" to match
the assertion value and the stack definition; locate the assertion referencing
duration_alarm['Threshold'] inside the test (in
test_expiration_reminder_stack.py) and change the preceding comment text from
"10 minutes (600,000 milliseconds)" to "14 minutes (840,000 milliseconds)" so
the comment and the assert(840_000) are consistent.
In `@backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py`:
- Around line 344-355: The current post-invoke check uses
response.get('FunctionError') after calling lambda_client.invoke with
InvocationType='Event' (async), but FunctionError is never set for async
invocations; replace this dead check by validating the invocation HTTP status
code (response.get('StatusCode') == 202) and raise SmokeTestFailureException if
it's not 202, or if you prefer keeping defensive commentary, remove the
FunctionError block and add a comment near lambda_client.invoke noting that
async invokes return 202 with no payload. Reference: lambda_client.invoke,
InvocationType='Event', response.get('FunctionError'),
response.get('StatusCode'), SmokeTestFailureException, lambda_name, event.
In `@backend/multi-account/backups/requirements.txt`:
- Around line 11-27: Check whether this upgrade path moves from a CDK version
earlier than v2.237.0 and, if so, audit any code referencing the IAM interface
IEncryptedResource: update usages that assumed IEncryptedResource implemented
IResource to instead handle the new requirement for IEnvironmentAware (e.g.,
replace casts or interface checks against IResource with IEnvironmentAware,
adjust implementations of classes or factories that returned IEncryptedResource
to also implement IEnvironmentAware, and run build/tests to surface type
errors). Also confirm other bumped packages (aws-cdk-asset-awscli-v1,
constructs, jsii) do not require code changes.
🧹 Nitpick comments (16)
backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py (1)
227-237: Stale:param compact_abbreviations:in docstring.
_configure_access_policiesno longer takes any parameters, but line 236 still documents:param compact_abbreviations:. Remove it to keep docs in sync.Proposed fix
def _configure_access_policies(self): """ Configure access policies for the OpenSearch domain. Creates IAM-based access policies that restrict access to specific Lambda roles: - Ingest role: POST/PUT access to compact indices - Index manager role: GET/HEAD/POST/PUT access for index management - Search API role: POST access restricted to _search endpoint only - - :param compact_abbreviations: List of compact abbreviations for index access policies """backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
29-62: Method name inconsistency with compact-connect counterpart.This mixin's method is named
correct_expired_license_status(Line 42), but the identical mixin inbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.pyusescheck_for_stale_license_status(Line 42). While both names are fine individually, the inconsistency between the two copies could cause confusion during maintenance.Consider aligning the method name with the compact-connect version (
check_for_stale_license_status) or vice versa.backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts (1)
2-2: Redundantaws-sdk-client-mock-jestimport.This side-effect import is now handled globally by
tests/jest.setup.ts. Removing it from individual test files would reduce duplication. Not urgent since it's idempotent.backend/compact-connect/lambdas/nodejs/tests/lib/email/investigation-notification-service.test.ts (1)
2-2: Same redundantaws-sdk-client-mock-jestimport as in the encumbrance test file.Can be removed since
jest.setup.tsnow handles this globally.backend/compact-connect/lambdas/nodejs/tests/lib/email/ingest-event-email-service.test.ts (1)
2-2: Same redundantaws-sdk-client-mock-jestimport — can be cleaned up across all test files.Since this applies to multiple test files, consider a follow-up to remove the now-unnecessary
import 'aws-sdk-client-mock-jest'lines from all individual test files that still have them..github/workflows/check-multi-account.yml (1)
27-29: LGTM — pip upgrade steps consistent with other workflow files.Same CVE-mitigation pattern applied to both LintPython and TestApps jobs.
Tangential note: this workflow still uses
actions/checkout@v2(lines 25, 45) while other workflows in this PR use@v5. Consider updating in a follow-up for consistency and to pick up security/performance improvements in newer versions.Also applies to: 52-54
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
102-104: Good fix — removed PII from the FunctionError log path.The generic
except Exceptionhandler at line 110 still logs the fullpayload, which can contain provider emails/names. Consider applying the same redaction there.🔒 Suggested redaction for the generic exception handler
except Exception as e: error_message = f'Error invoking email notification service lambda: {str(e)}' - self._logger.error(error_message, payload=payload, exception=str(e)) + self._logger.error(error_message) raise CCInternalException(error_message) from eBased on learnings: avoid logging full request bodies and redact sensitive fields (PII) by default.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
8-25: Date formatting helpers lack input validation.Both
formatIsoDateForDisplayandformatIsoDateAsSlashFormatwill silently produce"undefined ..."if given a malformed ISO string (e.g., missing parts or out-of-range month). Since these are internal helpers fed by controlled Python output, the risk is low, but a guard could prevent confusing emails if unexpected data slips through.🛡️ Optional: add a guard for malformed input
function formatIsoDateForDisplay(isoDate: string): string { const [year, month, day] = isoDate.split('-').map(Number); const monthName = MONTH_NAMES[month - 1]; + + if (!monthName || !year || !day) { + throw new Error(`Invalid ISO date string: ${isoDate}`); + } return `${monthName} ${day}, ${year}`; }backend/compact-connect/tests/app/base.py (1)
600-651: Consider addingexpiration_reminder_stackto resource count monitoring.The
_check_backend_stage_resource_countsmethod monitors resource counts for all stacks but doesn't include the newexpiration_reminder_stack. Since this method provides early warnings when stacks approach CloudFormation's 500-resource limit, it would be worth adding it alongside the other conditionally-present stacks.Proposed addition
if stage.persistent_stack.hosted_zone: stacks_to_check.extend( [ ('notification_stack', stage.notification_stack), ('reporting_stack', stage.reporting_stack), ] ) + if stage.environment_name != 'beta': + stacks_to_check.append(('expiration_reminder_stack', stage.expiration_reminder_stack))backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py (1)
424-428: Progress logging may never fire due to timing granularity.
int(elapsed) % 60 == 0relies onelapsedlanding exactly on a multiple of 60 seconds, but the loop sleeps for 10 seconds and log processing adds variable time, soint(elapsed)will often skip exact multiples. This means "Still waiting..." messages may appear erratically or not at all.Use a last-logged timestamp instead
+ last_progress_log = time.time() while time.time() - last_activity_time <= inactivity_timeout_sec: # ... existing loop body ... time.sleep(check_interval) - elapsed = time.time() - start_time - if int(elapsed) % 60 == 0: # Log every minute - logger.info(f'Still waiting for Lambda completion... ({int(elapsed)}s elapsed)') + elapsed = time.time() - start_time + if time.time() - last_progress_log >= 60: + logger.info(f'Still waiting for Lambda completion... ({int(elapsed)}s elapsed)') + last_progress_log = time.time()backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
398-401: Minor:formattedExpirationDatein the local cast type but not in thePrivilegeExpirationReminderRowinterface.The local cast type includes
formattedExpirationDate?: string, but thePrivilegeExpirationReminderRowinterface (defined inemail-notification-service.ts, Lines 29–34) does not declare this field. If the email service method readsformattedExpirationDatefrom the row, it would work at runtime via structural typing but lacks type safety. Consider adding it as an optional field on the interface for consistency.backend/compact-connect/lambdas/python/search/tests/function/test_expiration_reminders.py (2)
500-527: Verify decorator-to-parameter mapping for thecurrent_standard_datetimepatch.The
@patchwith a replacement value (Line 503) does not inject a positional mock argument, so the three remaining@patchdecorators map bottom-to-top as:mock_iter←iterate_privileges_by_expiration_date,mock_tracker_class←ExpirationReminderTracker,mock_email_client←email_service_client. This is correct. Just flagging that the ordering can be easy to get wrong with 4 stacked decorators — a brief inline comment noting the non-injected patch could help future readers.
529-572: Side-effect list over-provisioned.
mock_tracker_instance.was_already_sent.side_effectat Line 538 has 5 values ([False, True, True, False, False]) but only 3 providers are yielded, so only the first 3 values are consumed. The trailingFalse, Falseare unused. This isn't a bug, but trimming to[False, True, True]would make the test intent clearer.backend/compact-connect/lambdas/nodejs/lib/email/base-email-service.ts (1)
334-371: Consider extracting the shared logo block into a reusable helper.The logo
Imageblock definition is now duplicated across three methods:insertHeaderWithJurisdiction(line 183),insertHeader(line 286), and the newinsertLogo(line 346). A small private helper that creates and registers the logo block would reduce this duplication.backend/compact-connect/lambdas/python/search/expiration_reminder_tracker.py (1)
109-139: Minor: noupdatedAttimestamp on the record.The written item includes a TTL but no human-readable timestamp for when it was created or last updated. This could help with debugging/auditing without having to reverse-engineer the TTL. Consider adding a field like
'updatedAt': datetime.now(tz=timezone.utc).isoformat().backend/compact-connect/lambdas/python/search/handlers/expiration_reminders.py (1)
118-120: Nit:from Noneis unnecessary outside anexceptblock.
from Noneis typically used insideexceptblocks to suppress implicit exception chaining. Here on line 120, there's no active exception context, sofrom Nonehas no effect. It's harmless but slightly misleading.Suggested fix
- raise RuntimeError(f'Exceeded maximum continuation depth of {MAX_CONTINUATION_DEPTH}') from None + raise RuntimeError(f'Exceeded maximum continuation depth of {MAX_CONTINUATION_DEPTH}')
backend/compact-connect/lambdas/python/cognito-backup/requirements.txt
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py
Show resolved
Hide resolved
backend/compact-connect/tests/app/test_expiration_reminder_stack.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py
Outdated
Show resolved
Hide resolved
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Looking good- left a few comments/questions!
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/search/tests/function/test_expiration_reminders.py
Show resolved
Hide resolved
- reworded GH action - improved tests verification/comments for clarity - add details to schema docs
✅ Actions performedReviews resumed. |
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 (1)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
108-111:⚠️ Potential issue | 🟠 MajorPayload still logged in the generic
exceptpath (PII risk).Line 104 was correctly updated to omit the payload, but this
exceptbranch at line 110 still logspayload=payload, which can contain provider emails and names. Apply the same redaction here for consistency.🔒 Suggested fix
except Exception as e: error_message = f'Error invoking email notification service lambda: {str(e)}' - self._logger.error(error_message, payload=payload, exception=str(e)) + self._logger.error(error_message, exception=str(e)) raise CCInternalException(error_message) from eBased on learnings: avoid logging full request bodies and redact sensitive fields (PII) by default.
🤖 Fix all issues with AI agents
In `@backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py`:
- Around line 371-419: The log polling loop using logs_client.filter_log_events
currently only processes the first page (limit=100) and never consumes
response.get('nextToken'), so the "Completed processing" message can be missed;
update the polling block around filter_log_events to paginate through all pages
each cycle by using nextToken and looping until it's None (or set
interleaved=False and iterate pages), avoid reusing the name event to prevent
shadowing the outer dict (rename loop variable to log_event), and ensure
seen_event_ids, last_activity_time and the existing completion/metrics
extraction logic use the paginated log_event items.
🧹 Nitpick comments (5)
backend/compact-connect/stacks/search_persistent_stack/provider_search_domain.py (1)
227-264: Stale:paramin docstring.Line 236 still documents
:param compact_abbreviations:but the method no longer accepts any parameters. Minor docstring drift.Suggested fix
def _configure_access_policies(self): """ Configure access policies for the OpenSearch domain. Creates IAM-based access policies that restrict access to specific Lambda roles: - Ingest role: POST/PUT access to compact indices - Index manager role: GET/HEAD/POST/PUT access for index management - Search API role: POST access restricted to _search endpoint only - - :param compact_abbreviations: List of compact abbreviations for index access policies """backend/compact-connect/lambdas/python/search/tests/function/test_expiration_reminders.py (1)
638-708: Minor: thecurrent_standard_datetimepatch on line 641 is unnecessary here.Since
_make_eventsupplies an explicittargetDate, the handler won't fall through to thetoday + daysBeforecalculation path. The patch is harmless but could mislead readers into thinking this test exercises the date-calculation branch.backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py (1)
428-430: Periodic progress log fires unreliably.
int(elapsed) % 60 == 0depends on the integer elapsed seconds being exactly divisible by 60 at the moment of the check. Withcheck_interval=10plus variable loop-body time, the integer value can skip over 60 (e.g., 59 → 71), causing the log to never fire or fire inconsistently.A simple counter or threshold approach is more reliable:
Suggested fix
+ next_progress_log = 60 # seconds while time.time() - last_activity_time <= inactivity_timeout_sec: ... time.sleep(check_interval) elapsed = time.time() - start_time - if int(elapsed) % 60 == 0: # Log every minute + if elapsed >= next_progress_log: logger.info(f'Still waiting for Lambda completion... ({int(elapsed)}s elapsed)') + next_progress_log += 60backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
51-59: No error handling for malformeddateOfExpirationstrings.
date.fromisoformat(date_of_expiration)on Line 57 will raiseValueErrorif the string isn't a valid ISO date. In apre_loadhook, this would propagate as an unhandled exception rather than a marshmallowValidationError, potentially causing a 500 instead of a validation failure. If the data can ever be malformed (e.g., OpenSearch corruption), consider wrapping in a try/except that either returnsin_dataunchanged or raisesValidationError.Defensive parsing suggestion
# Parse the expiration date (handle both string and date objects) if isinstance(date_of_expiration, str): - expiration_date = date.fromisoformat(date_of_expiration) + try: + expiration_date = date.fromisoformat(date_of_expiration) + except ValueError: + return in_data else: expiration_date = date_of_expirationbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/api.py (1)
25-60: Consider extracting a shared base mixin to reduce duplication withLicenseExpirationStatusMixin.
PrivilegeExpirationStatusMixinandLicenseExpirationStatusMixin(inlicense/api.py) are nearly identical — they differ only in the status field name ('status'vs'licenseStatus') and method name. A single parameterized mixin in a shared location (e.g.,schema/common.py) would eliminate this duplication.Sketch of a unified mixin
class ExpirationStatusMixin: """Corrects stale active status when dateOfExpiration has passed.""" _status_field_name: str # Override in subclass or set via __init_subclass__ `@pre_load` def check_for_stale_status(self, in_data, **kwargs): if in_data.get(self._status_field_name) != ActiveInactiveStatus.ACTIVE: return in_data date_of_expiration = in_data.get('dateOfExpiration') if date_of_expiration is None: return in_data if isinstance(date_of_expiration, str): expiration_date = date.fromisoformat(date_of_expiration) else: expiration_date = date_of_expiration if expiration_date < config.expiration_resolution_date: in_data[self._status_field_name] = ActiveInactiveStatus.INACTIVE return in_data class LicenseExpirationStatusMixin(ExpirationStatusMixin): _status_field_name = 'licenseStatus' class PrivilegeExpirationStatusMixin(ExpirationStatusMixin): _status_field_name = 'status'
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py
Outdated
Show resolved
Hide resolved
- Update cognito-backup module to use common-layer deps - update test comment
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py`:
- Around line 77-86: JURISDICTION_2 and LICENSE_TYPE_2 can end up identical to
JURISDICTION and LICENSE_TYPE causing create_provider_records to attempt
duplicate privilege rows; add a guard after those assignments to detect if
(JURISDICTION_2 == JURISDICTION and LICENSE_TYPE_2 == LICENSE_TYPE) and fail
fast (raise SmokeTestFailureException with a clear message) or choose a
different jurisdiction/license if available; reference the variables
JURISDICTION, JURISDICTION_2, LICENSE_TYPE, LICENSE_TYPE_2 and the
create_provider_records call so the check is placed before
create_provider_records runs.
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/expiration_reminder_load_tests.py (1)
423-426: Periodic "still waiting" log is unreliable and will rarely fire.
int(elapsed) % 60 == 0depends onelapsedbeing exactly a multiple of 60 when checked. Withcheck_interval = 10plus variable processing time,int(elapsed)will often skip over exact multiples (e.g., 59 → 61). This means the progress message almost never prints.Proposed fix — track last log time instead
start_time = time.time() last_activity_time = time.time() seen_event_ids = set() # Track which log events we've already processed + last_progress_log_time = start_time while time.time() - last_activity_time <= inactivity_timeout_sec: ... time.sleep(check_interval) elapsed = time.time() - start_time - if int(elapsed) % 60 == 0: # Log every minute + if time.time() - last_progress_log_time >= 60: logger.info(f'Still waiting for Lambda completion... ({int(elapsed)}s elapsed)') + last_progress_log_time = time.time()
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen This is ready to merge! Sorry for the delay.
4 participants
We want to notify practitioners when any of their privileges are expiring in 30 days, 7 days, and the last day the privilege is active. This requires a daily cron process to search all providers with any privilege expiring within one of those time periods, and sending them an email notification.
Requirements List
Description List
Testing List
Closes #1259
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores
Tests