Feat/faster upload feedback #1128
Conversation
WalkthroughRoutes the ingest-event-reporter Lambda by Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor EventBridge as EventBridge (15‑min Rule)
participant Lambda as ingest-event-reporter
participant EventClient as EventClient
participant Mailer as Email Service
EventBridge->>Lambda: invoke { eventType: "frequent" }
activate Lambda
Lambda->>EventClient: getLast15MinuteTimestamps()
EventClient-->>Lambda: [startTs, endTs]
Lambda->>Lambda: runFrequentReports(compactConfig, jurisdictionConfig)
alt Errors found in period
Lambda-->>Mailer: send report email (errors)
else No errors / no action
Note right of Lambda: no email sent
end
deactivate Lambda
sequenceDiagram
autonumber
actor EventBridge2 as EventBridge (Weekly Rule)
participant Lambda2 as ingest-event-reporter
participant Mailer2 as Email Service
EventBridge2->>Lambda2: invoke { eventType: "weekly" }
activate Lambda2
Lambda2->>Lambda2: runWeeklyReports(compactConfig, jurisdictionConfig)
alt Errors during week
Lambda2-->>Mailer2: send report email (errors)
else No errors and license updates present
Lambda2-->>Mailer2: send "All's well" email
else No license updates in week
Lambda2-->>Mailer2: send "No license updates" email
end
deactivate Lambda2
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/compact-connect/lambdas/nodejs/lib/event-client.ts (1)
183-188: Log message typo in getIngestSuccessesMessage says “Getting ingest failures”; should be “ingest successes” to avoid confusion.
- this.logger.info('Getting ingest failures', { + this.logger.info('Getting ingest successes', {backend/compact-connect/stacks/reporting_stack.py (1)
89-99: Verify alignment with “near‑instant” feedback objectiveIssue #1109 asks for immediate emails post‑upload; this implements 15‑minute cadence. If near‑instant is required, consider an EventBridge event pattern trigger on license.validation-error / license.ingest-failure to invoke the same Lambda for just the affected jurisdiction, with basic rate‑limiting.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
169-177: Reduce PII in logs for invalid recordsvalid_data can include names, licenseNumber, and NPI. Prefer logging only non‑PII (e.g., recordNumber, compact/jurisdiction, error summary), or mask/summarize the fields.
- logger.info( - 'Invalid license in line %s uploaded: %s', - i + 1, - str(e), - valid_data=report_license_data, - exc_info=e, - ) + logger.info( + 'Invalid license uploaded', + record_number=i + 1, + error=str(e), + compact=compact, + jurisdiction=jurisdiction, + )backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
83-90: Minor docstring nitFor a response schema, “Python -> dump() -> API” may be clearer than “load()”, unless intentionally using load for field whitelisting.
backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts (1)
21-27: Optional: rename NIGHTLY to FREQUENT for claritySAMPLE_NIGHTLY_EVENT triggers the frequent path via default. Consider renaming to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts(2 hunks)backend/compact-connect/lambdas/nodejs/lib/event-client.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/models/index.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts(3 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/event-client.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py(0 hunks)backend/compact-connect/stacks/reporting_stack.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (2)
webroot/src/network/mocks/mock.data.ts (1)
compactConfig(1440-1486)backend/compact-connect/lambdas/nodejs/lib/models/compact.ts (1)
Compact(6-17)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
ForgivingSchema(21-25)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (22)
providerId(28-29)providerId(160-161)compact(32-33)compact(164-165)jurisdiction(36-37)jurisdiction(168-169)licenseType(40-41)licenseType(172-173)licenseStatusName(120-121)licenseStatus(116-117)jurisdictionUploadedLicenseStatus(124-125)compactEligibility(132-133)jurisdictionUploadedCompactEligibility(128-129)npi(44-45)licenseNumber(48-49)givenName(56-57)middleName(60-61)familyName(64-65)suffix(68-69)dateOfIssuance(72-73)dateOfRenewal(76-77)dateOfExpiration(80-81)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (5)
Compact(48-50)Jurisdiction(53-55)ActiveInactive(58-60)CompactEligibility(63-65)NationalProviderIdentifier(43-45)
backend/compact-connect/lambdas/nodejs/tests/lib/event-client.test.ts (1)
backend/compact-connect/lambdas/nodejs/lib/event-client.ts (1)
EventClient(20-237)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (2)
LicensePostRequestSchema(25-80)LicenseReportResponseSchema(83-109)
backend/compact-connect/stacks/reporting_stack.py (1)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (1)
handler(59-93)
🪛 GitHub Actions: Check-Lambda-JS
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts
[error] 81-81: ESLint: Expected indentation of 16 spaces but found 20.
🪛 GitHub Check: CheckLambdas
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts
[failure] 87-87:
Expected indentation of 20 spaces but found 24
[failure] 86-86:
Expected indentation of 20 spaces but found 24
[failure] 85-85:
Expected indentation of 20 spaces but found 24
[failure] 84-84:
Expected indentation of 16 spaces but found 20
[failure] 83-83:
Expected indentation of 20 spaces but found 24
[failure] 82-82:
Expected indentation of 20 spaces but found 24
[failure] 81-81:
Expected indentation of 16 spaces but found 20
backend/compact-connect/lambdas/nodejs/lib/event-client.ts
[failure] 41-41:
Expected blank line before this statement
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (9)
backend/compact-connect/lambdas/nodejs/lib/models/index.ts (1)
3-3: Re-export looks goodThis exposes Compact via 'lib/models' and matches the Lambda import.
backend/compact-connect/stacks/reporting_stack.py (1)
89-96: 15‑minute schedule wired correctlyCron and target payload for frequent runs look good.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py (1)
130-172: Good: narrowed validData to report schemaSwitching to LicenseReportResponseSchema reduces PII exposure in emails while preserving useful fields.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/api.py (1)
83-110: Report schema addition looks appropriateField set matches the reduced report payload; forgiving schema is appropriate for sanitizing inputs.
backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts (2)
76-183: Tests align with the frequent pathAssertions match the new dispatch and frequent behavior.
462-530: Weekly “errors present” test expectations updated correctlyNegative assertions for all emails are consistent with weekly behavior.
backend/compact-connect/lambdas/nodejs/tests/lib/event-client.test.ts (1)
80-134: 15‑minute window tests are solidCoverage for typical and boundary cases looks good.
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (2)
95-132: Frequent path behavior looks correctUses last complete 15‑minute window, avoids overlap; good logging and recipients.
Please confirm that sending only on failures (and not successes) in the frequent path matches stakeholder expectations.
134-182: Weekly path logic consistent with requirementsAll’s‑well and no‑updates emails are dispatched as expected with correct recipients.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/nodejs/lib/event-client.ts (1)
34-58: Tighten getLast15MinuteTimestamps: add return type, use Math.floor, fix paddingAdd tuple return type and avoid string parsing; this also appeases the linter’s padding rule.
- public getLast15MinuteTimestamps() { + + public getLast15MinuteTimestamps(): [number, number] { @@ - return [ - Number.parseInt( - (last15MinuteBlockStart.valueOf()/1000).toString() - ), - Number.parseInt( - (last15MinuteBlockEnd.valueOf()/1000).toString() - ) - ]; + return [ + Math.floor(last15MinuteBlockStart.valueOf() / 1000), + Math.floor(last15MinuteBlockEnd.valueOf() / 1000), + ]; }backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (1)
80-89: Fix switch indentation and remove trailing semicolon; optionally guard per-jurisdiction errorsESLint reported indentation issues and a stray semicolon. Also, wrap each case to prevent one failure from halting all jurisdictions.
- switch (event.eventType) { - case 'weekly': - await this.runWeeklyReports(compactConfig, jurisdictionConfig); - break; - default: - // frequent case (every 15 minutes) - await this.runFrequentReports(compactConfig, jurisdictionConfig); - break; - }; + switch (event.eventType) { + case 'weekly': + try { + await this.runWeeklyReports(compactConfig, jurisdictionConfig); + } catch (err) { + logger.error('Weekly report failed for jurisdiction', { + compact: compactConfig.compactAbbr, + jurisdiction: jurisdictionConfig.postalAbbreviation, + error: err instanceof Error ? err.message : String(err), + }); + } + break; + default: + // frequent case (every 15 minutes) + try { + await this.runFrequentReports(compactConfig, jurisdictionConfig); + } catch (err) { + logger.error('Frequent report failed for jurisdiction', { + compact: compactConfig.compactAbbr, + jurisdiction: jurisdictionConfig.postalAbbreviation, + error: err instanceof Error ? err.message : String(err), + }); + } + break; + }
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (2)
95-132: LGTM on frequent path; add explicit return type for clarityBehavior matches objectives. Optional: annotate return type.
- public async runFrequentReports(compactConfig: Compact, jurisdictionConfig: IJurisdiction) { + public async runFrequentReports(compactConfig: Compact, jurisdictionConfig: IJurisdiction): Promise<void> {
134-182: Weekly path looks good; minor style/typing nitsAdd explicit return type and fix spacing in else-if; consider logging week window for parity.
- public async runWeeklyReports(compactConfig: Compact, jurisdictionConfig: IJurisdiction) { + public async runWeeklyReports(compactConfig: Compact, jurisdictionConfig: IJurisdiction): Promise<void> { @@ - } - else if(!weeklyIngestEvents.ingestSuccesses.length) { + } else if (!weeklyIngestEvents.ingestSuccesses.length) {Optionally include weekStartStamp/weekEndStamp in the success/warn logs for traceability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts(2 hunks)backend/compact-connect/lambdas/nodejs/lib/event-client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/nodejs/ingest-event-reporter/lambda.ts (2)
webroot/src/network/mocks/mock.data.ts (1)
compactConfig(1440-1486)backend/compact-connect/lambdas/nodejs/lib/models/compact.ts (1)
Compact(6-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
|
The Check-Python is failing because of the |
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
Great. Just two questions to make sure I'm not missing something
backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/nodejs/lib/event-client.ts (2)
69-72: LGTM!The change from
Number.parseIntwith string conversion toMath.flooris a cleaner approach that avoids unnecessary string parsing while maintaining the same truncation behavior for Unix timestamps.
88-91: LGTM!Consistent with the change in
getYesterdayTimestamps, usingMath.floorprovides a more direct approach for converting milliseconds to seconds.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts (1)
76-76: Align test event type with “Frequent runs”.The “Frequent runs” suite still uses
SAMPLE_NIGHTLY_EVENT(eventType: 'nightly'), which relies on the dispatcher’s default branch to exercise the frequent path. Rename the constant (e.g. toSAMPLE_FREQUENT_EVENT) and update itseventType(e.g. to'frequent') for clarity and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/nodejs/lib/event-client.ts(3 hunks)backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (4)
backend/compact-connect/lambdas/nodejs/tests/ingest-event-reporter.test.ts (3)
99-182: LGTM!The test correctly verifies that error report emails are sent immediately when validation errors or ingest failures occur in the frequent (15-minute) path, aligning with the PR's objective of providing faster feedback to states.
184-242: LGTM!The test correctly verifies that no emails are sent when there are no ingest events in the frequent path. This is appropriate since the frequent (15-minute) path focuses on immediate error notification rather than status updates.
462-530: LGTM!The updated test correctly reflects the new architecture where the weekly path no longer sends error report emails. This makes sense because errors are now reported immediately via the frequent (15-minute) path, eliminating the need for duplicate error reporting in the weekly summary.
backend/compact-connect/lambdas/nodejs/lib/event-client.ts (1)
29-54: LGTM!The implementation correctly calculates the last complete 15-minute block boundaries. The logic properly:
- Identifies the start of the current 15-minute block
- Uses that as the end boundary of the previous complete block
- Subtracts 15 minutes to get the start boundary
The method enables the 15-minute error reporting frequency as described in the PR objectives.
|
@jlkravitz , ready for you! |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen good to merge
4 participants
Description List
Testing List
Closes #1109
Summary by CodeRabbit
New Features
Improvements
Behavioral Changes
Tests
Privacy