Feat/aa notifications #864
Conversation
WalkthroughThis change implements comprehensive notification handling for license and privilege encumbrance and lifting events. It introduces new event listeners, Lambda handlers, notification services, and data model/schema updates to ensure both providers and all states where a provider holds a license or privilege are notified on encumbrance or lifting. Extensive automated tests and infrastructure updates support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant EventBridge
participant DataEventsLambda
participant DataClient
participant EventBusClient
participant EmailNotificationStack
participant EmailNotificationLambda
participant EncumbranceNotificationService
participant SES
EventBridge->>DataEventsLambda: License/Privilege Encumbrance Event
DataEventsLambda->>DataClient: Encumber/Lift privilege(s)
DataEventsLambda->>EventBusClient: Publish privilege encumbrance event(s)
EventBusClient->>EventBridge: Emit privilege encumbrance event(s)
EventBridge->>EmailNotificationStack: Notification Event (via SQS)
EmailNotificationStack->>EmailNotificationLambda: Trigger Lambda
EmailNotificationLambda->>EncumbranceNotificationService: Send notification (provider/state)
EncumbranceNotificationService->>SES: Send email(s)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
248-307:RET503suppression remains & explicit return still missingThe comment
# noqa: RET503was intended to be dropped but is still present.
More importantly, the function can return a dict on success yet falls through to_handle_api_errorwithout an explicitreturnafterwards, keeping the linter warning alive.- def void_unsettled_charge_on_credit_card( # noqa: RET503 + def void_unsettled_charge_on_credit_card( ... - self._handle_api_error(response) + self._handle_api_error(response) + return None # make return type explicitRepeat for
process_charge_on_credit_card_for_privilege_purchase.
484-502: Add explicitreturn Noneafter error pathSame RET503 issue in
process_charge_on_credit_card_for_privilege_purchase.- self._handle_api_error(response) + self._handle_api_error(response) + return NoneThis satisfies static analysis and clarifies intent.
🧹 Nitpick comments (17)
backend/compact-connect/common_constructs/ssm_parameter_utility.py (1)
34-35: Adopted Sphinx-style docstring annotations
The conversion from Google-styleArgs/Returnsto:param/:return:is appropriate. For completeness, consider adding a:rtype: EventBusannotation to the return description.backend/compact-connect/lambdas/nodejs/lib/email/cognito-email-service.ts (1)
63-72: Repeated literal args – extract a helper?Every call now passes identical alignment/markdown flags:
this.insertBody(template, text, 'center', true)Consider a tiny helper (e.g.
this.insertCenteredMarkdown(template, text)) to prevent copy-paste and protect against signature drift.Also applies to: 90-96, 112-118, 136-141, 160-165, 184-189
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py (3)
140-149: Create-newEventBusClient()→ use the already-configured singletonInstantiating a fresh
EventBusClient()on every request (warm or cold) is unnecessary and makes unit tests harder because two objects need stubbing. Use the same instance placed onconfig(as done in the privilege path).- event_bus_client = EventBusClient() - event_bus_client.publish_license_encumbrance_event( + config.event_bus_client.publish_license_encumbrance_event(This also avoids extra connection overhead for high-volume flows.
193-201: Mirror the change for lifting eventsSame observations as above: align
sourcevalue and reuseconfig.event_bus_client.- source='provider-data-v1', + source='compact-connect.encumbrance',Consistency keeps CloudWatch queries & alerting rules simple.
245-254: Single event-bus client for license-lift path as wellFor the final branch, remove the per-call instantiation and rely on the shared client.
backend/compact-connect/lambdas/nodejs/lib/email/base-email-service.ts (1)
316-338: Optional: omitmarkdownprop when false to reduce payload size
insertBodynow always serialises"markdown": false, resulting in larger JSON and HTML for every non-markdown email.
If the email renderer treats absence andfalseidentically, avoid emitting the property when not needed:- 'props': { - 'text': bodyText, - 'markdown': markdown - } + 'props': { + 'text': bodyText + // 'markdown' added only when true + ... (markdown ? { markdown } : {}) + }Minor, but over thousands of emails it shaves bytes.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
49-71: Reduce duplication & align field namesAll four new schema classes repeat identical fields. Consider a mix-in to DRY things up:
class _EncumbranceBase: providerId = UUID(required=True, allow_none=False) licenseTypeAbbreviation = String(required=True, allow_none=False) class LicenseEncumbranceDetailSchema(_EncumbranceBase, DataEventDetailBaseSchema): effectiveStartDate = Date(required=True, allow_none=False)Also note we now have both
licenseTypeAbbrev(in older privilege schema) andlicenseTypeAbbreviation(new). Divergent naming complicates generic handlers.backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts (1)
76-85: Side-effects onprocess.envleak across test files
beforeEachsetsprocess.env.FROM_ADDRESS&UI_BASE_PATH_URLbut never restores them.
If another test suite relies on different values, this ordering dependency may bite.Minimal fix:
let originalEnv: NodeJS.ProcessEnv; beforeEach(() => { originalEnv = { ...process.env }; ... }); afterEach(() => { process.env = originalEnv; });backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
147-310: Consider extracting common validation logic to reduce duplication.The new encumbrance notification cases follow good patterns, but there's significant duplication in validation logic. Consider extracting common validation functions.
Here's a suggested approach to reduce duplication:
+// Add these helper functions before the handler method +private validateProviderNotificationVariables( + templateVariables: any, + requiredFields: string[] +): void { + const missing = requiredFields.filter(field => !templateVariables[field]); + if (missing.length > 0) { + throw new Error(`Missing required template variables: ${missing.join(', ')}`); + } +} + +private validateStateNotificationRequirements( + event: EmailNotificationEvent, + requiredVariables: string[] +): void { + if (!event.jurisdiction) { + throw new Error(`Missing required jurisdiction field for ${event.template} template.`); + } + this.validateProviderNotificationVariables(event.templateVariables, requiredVariables); +} // Then simplify each case, for example: case 'licenseEncumbranceProviderNotification': - if (!event.templateVariables.providerFirstName - || !event.templateVariables.providerLastName - || !event.templateVariables.encumberedJurisdiction - || !event.templateVariables.licenseType - || !event.templateVariables.effectiveStartDate) { - throw new Error('Missing required template variables for licenseEncumbranceProviderNotification template.'); - } + this.validateProviderNotificationVariables(event.templateVariables, [ + 'providerFirstName', 'providerLastName', 'encumberedJurisdiction', + 'licenseType', 'effectiveStartDate' + ]); await this.encumbranceService.sendLicenseEncumbranceProviderNotificationEmail( event.compact, event.specificEmails || [], event.templateVariables.providerFirstName, event.templateVariables.providerLastName, event.templateVariables.encumberedJurisdiction, event.templateVariables.licenseType, event.templateVariables.effectiveStartDate ); break;backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
2370-2388: Update the doc-string to reflect the new return valueThe method now returns
list[PrivilegeData], but the doc-string still describes only side-effects. This will trip up downstream users and type checkers.@@ - :param str license_type_abbreviation: The license type abbreviation + :param str license_type_abbreviation: The license type abbreviation + :return: List of `PrivilegeData` that were encumbered. An empty list + is returned when no privileges required updating. """
2461-2480: Synchronise doc-string with new signature
lift_home_jurisdiction_license_privilege_encumbrancesnow promises
-> list[PrivilegeData], yet the doc-string omits any return semantics.
Mirror the pattern suggested above so callers know they can rely on the
returned collection of updated privileges (or an empty list).backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
64-77: DuplicateAwsSolutions-IAM5suppressionsEvery helper adds the same wildcard IAM suppression snippet. After consolidating construction (see previous comment) you can keep a single suppression and avoid noise in cdk-nag output.
backend/compact-connect/tests/app/test_event_listener.py (1)
224-335: Parametrize duplicated testsThe four new tests repeat ~150 lines only varying
(listener, detail_type, handler_name). A@pytest.mark.parametrize(or a small helper that asserts queue/rule/mapping) would slash maintenance overhead and keep snapshots in sync with future listener additions.backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts (2)
50-61: Style violations – easy to fix before mergeSeveral lines exceed the 120-char lint limit flagged by CI (e.g., 53, 103, 150, 247 …). Run the formatter / linter to avoid failing the check-lambdas gate.
232-319: Heavy duplication across email buildersEight nearly identical methods differ only in template strings. Consider a small renderer that receives:
type EncumbranceCtx = { kind: 'license'|'privilege', action: 'encumbered'|'lifted', audience: 'provider'|'state', … };and returns subject/body + recipients, cutting 400+ lines and removing risk of divergent copy-paste bugs.
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
51-83: Docs stale afterevent_batch_writeraddition
publish_privilege_purchase_event(and others) gainedevent_batch_writer, but their docstrings were not updated. Update docstrings to prevent misuse and satisfy Sphinx / pydoc checks.
222-304: Argument bloat hints at missing abstraction
publish_privilege_encumbrance_event& siblings take 7-8 positional params and replicate the same build/validate/publish steps. A private_encumbrance_event(detail_schema, detail_type, **kwargs)helper would:
- slash repetitive code,
- satisfy pylint R0913/R0917,
- ensure future field changes stay consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
backend/compact-connect/common_constructs/ssm_parameter_utility.py(1 hunks)backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(5 hunks)backend/compact-connect/lambdas/nodejs/lib/email/base-email-service.ts(2 hunks)backend/compact-connect/lambdas/nodejs/lib/email/cognito-email-service.ts(6 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/index.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/ingest-event-email-service.test.ts(7 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/jurisdiction-client.test.ts(4 hunks)backend/compact-connect/lambdas/nodejs/tests/sample-records.ts(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(7 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(9 hunks)backend/compact-connect/lambdas/python/common/cc_common/license_util.py(2 hunks)backend/compact-connect/lambdas/python/common/common_test/test_constants.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(0 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/common/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(5 hunks)backend/compact-connect/lambdas/python/data-events/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(11 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(4 hunks)backend/compact-connect/lambdas/python/purchases/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/purchases/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt(2 hunks)backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt(2 hunks)backend/compact-connect/requirements-dev.txt(5 hunks)backend/compact-connect/requirements.txt(1 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(2 hunks)backend/compact-connect/tests/app/test_event_listener.py(1 hunks)backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py(2 hunks)backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py(1 hunks)backend/multi-account/control-tower/requirements.txt(1 hunks)backend/multi-account/log-aggregation/requirements.txt(1 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/common/cc_common/license_util.py (2)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1357-1490
Timestamp: 2025-05-30T13:57:36.286Z
Learning: The `LicenseUtility.get_license_type_by_abbreviation` method returns `None` when given an invalid license type abbreviation, rather than raising an exception like KeyError or ValueError.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#798
File: backend/compact-connect/lambdas/python/common/cc_common/license_util.py:36-46
Timestamp: 2025-05-14T16:26:16.829Z
Learning: In the `get_valid_license_type_abbreviations` method in `license_util.py`, KeyErrors should not be caught as they indicate an invalid compact value, which is intended to fail explicitly rather than silently returning an empty set.
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
[refactor] 49-49: Too few public methods (0/2)
(R0903)
[refactor] 55-55: Too few public methods (0/2)
(R0903)
[refactor] 61-61: Too few public methods (0/2)
(R0903)
[refactor] 67-67: Too few public methods (0/2)
(R0903)
backend/compact-connect/lambdas/python/purchases/purchase_client.py
[refactor] 248-248: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 309-309: Too many arguments (7/5)
(R0913)
[refactor] 309-309: Too many positional arguments (7/5)
(R0917)
[refactor] 309-309: Too many local variables (29/15)
(R0914)
[refactor] 309-309: Too many statements (84/50)
(R0915)
[refactor] 309-309: Either all return statements in a function should return an expression, or none of them should.
(R1710)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
[refactor] 220-220: Too many arguments (8/5)
(R0913)
[refactor] 220-220: Too many positional arguments (8/5)
(R0917)
[refactor] 257-257: Too many arguments (9/5)
(R0913)
[refactor] 257-257: Too many positional arguments (9/5)
(R0917)
[refactor] 297-297: Too many arguments (8/5)
(R0913)
[refactor] 297-297: Too many positional arguments (8/5)
(R0917)
[refactor] 334-334: Too many arguments (9/5)
(R0913)
[refactor] 334-334: Too many positional arguments (9/5)
(R0917)
[refactor] 374-374: Too many arguments (8/5)
(R0913)
[refactor] 374-374: Too many positional arguments (8/5)
(R0917)
[refactor] 411-411: Too many arguments (9/5)
(R0913)
[refactor] 411-411: Too many positional arguments (9/5)
(R0917)
[refactor] 451-451: Too many arguments (8/5)
(R0913)
[refactor] 451-451: Too many positional arguments (8/5)
(R0917)
[refactor] 488-488: Too many arguments (9/5)
(R0913)
[refactor] 488-488: Too many positional arguments (9/5)
(R0917)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
[refactor] 84-84: Too many arguments (6/5)
(R0913)
[refactor] 84-84: Too many positional arguments (6/5)
(R0917)
[refactor] 111-111: Too many arguments (6/5)
(R0913)
[refactor] 111-111: Too many positional arguments (6/5)
(R0917)
[refactor] 222-222: Too many arguments (8/5)
(R0913)
[refactor] 222-222: Too many positional arguments (8/5)
(R0917)
[refactor] 264-264: Too many arguments (8/5)
(R0913)
[refactor] 264-264: Too many positional arguments (8/5)
(R0917)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
[refactor] 121-121: Too many local variables (19/15)
(R0914)
[refactor] 247-247: Too many local variables (19/15)
(R0914)
[refactor] 375-375: Too many local variables (19/15)
(R0914)
[refactor] 501-501: Too many local variables (19/15)
(R0914)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
[error] 870-870: Too many positional arguments for function call
(E1121)
[error] 959-959: Too many positional arguments for function call
(E1121)
[error] 1046-1046: Too many positional arguments for function call
(E1121)
[error] 1129-1129: Too many positional arguments for function call
(E1121)
[error] 1178-1178: Too many positional arguments for function call
(E1121)
[error] 1285-1285: Too many positional arguments for function call
(E1121)
[error] 1375-1375: Too many positional arguments for function call
(E1121)
[error] 1468-1468: Too many positional arguments for function call
(E1121)
[error] 1559-1559: Too many positional arguments for function call
(E1121)
[error] 1642-1642: Too many positional arguments for function call
(E1121)
[error] 1683-1683: Too many positional arguments for function call
(E1121)
[error] 1691-1691: Too many positional arguments for function call
(E1121)
[error] 1776-1776: Too many positional arguments for function call
(E1121)
[error] 1865-1865: Too many positional arguments for function call
(E1121)
[error] 1952-1952: Too many positional arguments for function call
(E1121)
[error] 2035-2035: Too many positional arguments for function call
(E1121)
[error] 2084-2084: Too many positional arguments for function call
(E1121)
[error] 2191-2191: Too many positional arguments for function call
(E1121)
[error] 2281-2281: Too many positional arguments for function call
(E1121)
[error] 2374-2374: Too many positional arguments for function call
(E1121)
[error] 2465-2465: Too many positional arguments for function call
(E1121)
[error] 2548-2548: Too many positional arguments for function call
(E1121)
[error] 2593-2593: Too many positional arguments for function call
(E1121)
[error] 2601-2601: Too many positional arguments for function call
(E1121)
🪛 GitHub Check: CheckLambdas
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
[failure] 734-734:
This line has a length of 125. Maximum allowed is 120
[failure] 692-692:
Expected blank line before this statement
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts
[failure] 396-396:
This line has a length of 127. Maximum allowed is 120
[failure] 346-346:
This line has a length of 129. Maximum allowed is 120
[failure] 297-297:
This line has a length of 135. Maximum allowed is 120
[failure] 247-247:
This line has a length of 137. Maximum allowed is 120
[failure] 200-200:
This line has a length of 127. Maximum allowed is 120
[failure] 150-150:
This line has a length of 129. Maximum allowed is 120
[failure] 103-103:
This line has a length of 135. Maximum allowed is 120
[failure] 53-53:
This line has a length of 137. Maximum allowed is 120
🪛 Ruff (0.11.9)
backend/compact-connect/lambdas/python/purchases/purchase_client.py
307-307: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
501-501: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (31)
backend/compact-connect/lambdas/python/purchases/requirements.txt (1)
9-9: Approve bump of certifi dependency
Updatingcertifito 2025.6.15 brings in the latest security fixes and aligns with the version used in other lambda modules.backend/compact-connect/requirements-dev.txt (1)
15-93: Approve bump of development dependenciesThe pip-compiled dev requirements have been updated to the latest patch versions for certifi, coverage[toml], msgpack, pytest-cov, and ruff, and the pluggy annotation was expanded to list both pytest and pytest-cov. These changes align with the updates in other backend modules and adhere to the project’s dependency-pinning strategy. No issues detected.
backend/compact-connect/lambdas/python/common/requirements.txt (1)
9-11: Consistent boto3 and botocore version bump
The versions forboto3andbotocorehave been updated in lockstep to1.38.37, aligning with other lambda modules.backend/multi-account/control-tower/requirements.txt (1)
15-17: AWS CDK package version alignment
Upgradedaws-cdk-cloud-assembly-schemato44.5.0andaws-cdk-libto2.201.0, matching the multi-account and compact-connect modules for consistent CDK dependency versions.backend/multi-account/log-aggregation/requirements.txt (1)
15-17: Match AWS CDK dependency updates across stacks
Bumpedaws-cdk-cloud-assembly-schemaandaws-cdk-libto versions44.5.0and2.201.0, respectively, ensuring consistency with control-tower and other stacks.backend/compact-connect/lambdas/nodejs/tests/sample-records.ts (2)
273-273: Standardize jurisdictionName casing
Updated'jurisdictionName': 'Ohio'to use capitalized form, aligning with naming conventions in other tests and modules.
316-316: Unmarshalled sample consistency
Mirroring the raw record change, the unmarshalled object now correctly capitalizes'jurisdictionName': 'Ohio'.backend/compact-connect/lambdas/python/common/common_test/test_constants.py (1)
3-3: Validate new DEFAULT_EFFECTIVE_DATE constant.
The addedDEFAULT_EFFECTIVE_DATEsupports date-only scenarios in encumbrance event tests. Confirm it's referenced consistently in all related test cases.backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt (1)
7-9: Approve dependency version bumps.
boto3, botocore, certifi, and moto have been updated to 1.38.37, 1.38.37, 2025.6.15, and 5.1.6 respectively, matching the rest of the backend lambdas.Also applies to: 14-14, 36-36
backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt (1)
7-9: Approve dependency version bumps.
The staff-users lambda dependencies boto3, botocore, certifi, and moto have been aligned to 1.38.37, 1.38.37, 2025.6.15, and 5.1.6 to maintain consistency.Also applies to: 14-14, 42-42
backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt (1)
7-9: Approve dependency version bumps.
Upgraded boto3, botocore, certifi, and moto to 1.38.37, 1.38.37, 2025.6.15, and 5.1.6 in line with other lambdas.Also applies to: 14-14, 36-36
backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt (1)
7-9: Approve dependency version bumps.
Consistent updates applied: boto3 → 1.38.37, botocore → 1.38.37, certifi → 2025.6.15, moto → 5.1.6 across custom-resources lambda.Also applies to: 14-14, 36-36
backend/compact-connect/lambdas/python/data-events/requirements-dev.txt (1)
7-9: Approve bump of dependencies to align with project standards.Versions of
boto3,botocore,certifi, andmotohave been updated correctly and are consistent with other Python lambda modules. These changes support the improved notification workflows.Also applies to: 14-14, 36-36
backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt (1)
7-7: Approve bump of certifi dependency.The
certifiversion update aligns with other lambda modules and ensures consistent security certificates across the project.backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt (1)
7-9: Approve development dependency bumps.The updates to
boto3,botocore,certifi, andmotofor theprovider-data-v1lambda are consistent with other components, supporting the new encumbrance event tests and workflows.Also applies to: 14-14, 38-38
backend/compact-connect/requirements.txt (1)
15-19: Approve AWS CDK dependency upgrades.The AWS CDK libraries have been bumped to compatible versions, ensuring the new event listener stack constructs compile correctly with CDK v2.201.0.
Also applies to: 26-26
backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py (1)
175-179: Review test card number update.The test credit card number was updated to
'4007000000000027'. Please confirm this number is valid in the sandbox/test environment per Authorize.net testing guidelines and update documentation if necessary.backend/compact-connect/lambdas/python/purchases/requirements-dev.txt (1)
7-14: Dependency bumps look fine – please confirm cross-module consistency & CI layer rebuildThe new
boto3 / botocore / certifi / motopins are reasonable, but a mismatch between Lambda layers (or otherrequirements-dev.txtfiles) easily leads to hard-to-trace runtime errors.A quick sanity-check script:
#!/usr/bin/env bash # List every distinct pinned version for the upgraded libs. for pkg in boto3 botocore certifi moto; do echo "=== $pkg ===" rg -o --no-filename "$pkg==[0-9\.]+" | sort -u doneThis should print exactly one version per package. If multiple versions appear, please align them (or move to a shared constraints file).
Also applies to: 36-36
backend/compact-connect/lambdas/python/common/requirements-dev.txt (1)
7-14: Same version bumps – keep the whole fleet consistentGood to see the same versions here. Make sure the verification script above shows identical pins across all Lambda folders to avoid drift.
Also applies to: 38-38
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
192-192: ```shell
#!/usr/bin/env bashDouble-check for any stale calls to insertMarkdownBody
rg 'insertMarkdownBody(' -n
</details> <details> <summary>backend/compact-connect/lambdas/nodejs/tests/lib/email/ingest-event-email-service.test.ts (1)</summary> `49-50`: **Capitalisation update aligns tests with production data** Switching `'ohio'` → `'Ohio'` in input and expectation keeps the test suite faithful to UI-visible formatting. Looks good. Also applies to: 64-65, 86-87, 112-113, 131-132, 142-143, 162-163 </details> <details> <summary>backend/compact-connect/lambdas/nodejs/lib/email/index.ts (1)</summary> `3-3`: **Barrel export added – verify path correctness in build** `EncumbranceNotificationService` is now exposed. Ensure the file `./encumbrance-notification-service` exists at compile time and exports the class as default/named exactly as here. </details> <details> <summary>backend/compact-connect/lambdas/nodejs/tests/lib/jurisdiction-client.test.ts (2)</summary> `50-52`: **Capitalisation change looks good but double-check production data** The mocks now store `jurisdictionName` in Title-case (“Ohio”, “Nebraska”). If our production Dynamo rows still contain lowercase values you will have a false-positive test pass and a runtime mismatch. Please confirm the column is capitalised in real data (or normalise in `JurisdictionClient`). Also applies to: 118-119 --- `199-202`: **Assertion updated correctly** `arrayContaining(['Ohio', 'Nebraska'])` keeps the order-agnostic assertion – nice touch. No further issues here. </details> <details> <summary>backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts (1)</summary> `533-602`: **Mocks may be incomplete for new test case** `sendPrivilegePurchaseProviderNotificationEmail` can read compact configuration (logo URL, fee, etc.). The new tests don’t stub `mockCompactConfigurationClient` and rely on undefined behaviour. If implementation is later expanded your test will start throwing unexpectedly. Stub at least: ```ts mockCompactConfigurationClient.getCompactConfiguration.mockResolvedValue(SAMPLE_COMPACT_CONFIG);backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py (1)
164-172: Docstring update LGTMSwitch to Sphinx annotations improves generated docs – no functional impact.
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
11-11: LGTM! Clean separation of concerns.The addition of
EncumbranceNotificationServicefollows the existing pattern and maintains proper separation between different notification types.Also applies to: 25-25, 46-52
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
562-1118: Excellent test coverage for all encumbrance notification templates!The tests comprehensively cover all 8 new notification templates with both success and error scenarios. The test structure is consistent and follows best practices.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
261-293: Well-structured event publishing tests!The new tests properly verify that:
- Events are published with correct attributes (source, detail_type, and all required fields)
- Event publishing failures are handled appropriately by propagating exceptions
The test structure follows established patterns and provides good coverage for the event publishing functionality.
Also applies to: 815-849
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
1096-1105: Keep the exception contract explicitThe helper now simply forwards to
LicenseUtility.get_license_type_by_abbreviation, relying on that utility to raiseCCInvalidRequestExceptionfor bad input.
IfLicenseUtilityever changes (or raises a different exception), callers of_validate_license_type_abbreviationwill silently receive the wrong exception type and break the public contract documented a few lines above.Consider wrapping the call so we always raise the expected domain‐specific exception:
- return LicenseUtility.get_license_type_by_abbreviation(compact, license_type_abbreviation).name + try: + return LicenseUtility.get_license_type_by_abbreviation( + compact, license_type_abbreviation + ).name + except CCInvalidRequestException: + raise # pass through unchanged + except Exception as exc: + # Normalise anything unexpected + raise CCInvalidRequestException(str(exc)) from excbackend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
813-835: Looks good – helper builders keep tests readableUtility methods
_generate_privilege_encumbrance_message/_generate_privilege_encumbrance_lifting_messagemirror the earlier license helpers and make later tests concise.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/event_listener_stack/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
991-1033: Duplicate of provider-notification commentSame duplication & refactor opportunity as noted for lines 561-602.
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (4)
561-602: Extract common “provider notification” test logic into a helper / parameterised suiteThe four “*_Provider Notification” blocks that start here are structurally identical (build sample event → call
lambda.handler→ assert SES params).
Duplicated setup & assertions bloat the file (≈ 250 LOC) and make future template additions error-prone.Consider:
- describe('License Encumbrance Provider Notification', () => { - const SAMPLE_EVENT = { … } - it('should successfully send …', async () => { … }); - … - }); + describe.each` + template | subjectContains + ${'licenseEncumbranceProviderNotification'}| ${'license in Ohio is encumbered'} + ${'licenseEncumbranceLiftingProviderNotification'}|${'license in Ohio is no longer encumbered'} + ${'privilegeEncumbranceProviderNotification'}|${'privilege in Ohio is encumbered'} + ${'privilegeEncumbranceLiftingProviderNotification'}|${'privilege in Ohio is no longer encumbered'} + `('$template', ({ template, subjectContains }) => { + const EVENT = buildProviderEvent(template); + + it('sends email', async () => { + await lambda.handler(EVENT, {} as any); + expect(mockSESClient).toHaveReceivedCommandWith(SendEmailCommand, { + Destination: { ToAddresses: ['[email protected]'] }, + Message: { Subject: { Data: subjectContains } } + }); + }); + + it('throws on missing vars', async () => { + await expect(lambda.handler({ …EVENT, templateVariables: {} }, {} as any)) + .rejects.toThrow(`Missing required template variables for ${template} template.`); + }); + }); + + function buildProviderEvent(template: string): EmailNotificationEvent { … }Benefits: 70 % size reduction, single-point changes, clearer intent.
No functional behaviour changes – purely test maintainability.
674-693: Useat(-1)instead of index0when inspecting SES callsThe test assumes the handler sends exactly one e-mail and accesses:
const emailData = mockSESClient.commandCalls(SendEmailCommand)[0].args[0].input.Message?.Body?.Html?.Data;If another code path inside the same invocation triggers an additional
SendEmailCommandin the future, this assertion will silently start validating the wrong e-mail.Small safeguard:
-const emailData = mockSESClient.commandCalls(SendEmailCommand)[0].args[0].input.Message?.Body?.Html?.Data; +const calls = mockSESClient.commandCalls(SendEmailCommand); +const emailData = calls.at(-1)?.args[0].input.Message?.Body?.Html?.Data;Keeps the intent while remaining resilient to extra calls.
776-832: Repeated inlinecallsFakemocks – factor into utilityEach state-level notification (
license*StateNotification,privilege*StateNotification) duplicates the pattern:mockDynamoDBClient.on(GetItemCommand).callsFake((input) => { if (input.Key.sk.S === 'aslp#JURISDICTION#ca') { … } return Promise.resolve({ Item: SAMPLE_COMPACT_CONFIGURATION }); });Centralising this into
mockJurisdictionLookup('ca', customConfig)(or even a reusable Jest mock factory) would:
- Eliminate copy/paste errors,
- Make intent clearer (
mockJurisdictionLookupinstantly conveys purpose),- Simplify future additions (just pass jurisdiction code).
A tiny helper pays for itself quickly in a 1 100-line test file.
1047-1126: File size creeping past 1 100 LOC – consider splitting by featureWith the new encumbrance tests the file reaches ~1 120 lines, mixing settlement reports, privilege purchases, and encumbrances.
Breaking it up (e.g.email-notification-service/settlement.test.ts,…/privilege.test.ts,…/encumbrance.test.ts) would:
- Reduce cognitive load when scanning,
- Minimise merge conflicts,
- Allow Jest to run subsets faster via
--testPathPattern.No urgency, but worth considering as notification matrix grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts
- backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (1)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
604-613: Align error-message punctuation across testsThis block expects
'Missing required template variables for licenseEncumbranceProviderNotification template.'(trailing period).
Earlier tests (CompactTransactionReporting/JurisdictionTransactionReporting) omit the period, resulting in inconsistent contract checking.Please verify the actual message thrown by the lambda and update either:
- The implementation (preferred – add trailing period everywhere), or
- These assertions (remove the period).
Consistency avoids brittle tests and conveys a uniform developer experience.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts (1)
15-15: Address the null/undefined check for notification emails.This issue was flagged in a previous review and still needs to be resolved. The code assumes
jurisdictionAdverseActionsNotificationEmailsis always defined before calling.length.Apply this guard to prevent runtime errors:
-const recipients = jurisdictionConfig.jurisdictionAdverseActionsNotificationEmails; +const recipients = jurisdictionConfig.jurisdictionAdverseActionsNotificationEmails ?? [];backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
304-450: Consider further refactoring to reduce remaining duplication.While the helper functions significantly reduce code duplication, the four notification handlers (
privilege_encumbrance_listener,privilege_encumbrance_lifting_listener,license_encumbrance_notification_listener,license_encumbrance_lifting_notification_listener) still follow very similar patterns.Consider creating a generic notification handler that accepts the notification method as a parameter to eliminate the remaining ~120 lines of duplicated notification flow logic.
🧹 Nitpick comments (3)
backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
135-192: Good consolidation, but consider reducing parameter count.The
_add_emailer_event_listenerhelper method successfully addresses the code duplication issue mentioned in past reviews. However, the method has 7 parameters, which exceeds the typical limit of 5.Consider grouping related parameters into a configuration object:
+@dataclass +class EmailerListenerConfig: + construct_id_prefix: str + index: str + handler: str + listener_detail_type: str -def _add_emailer_event_listener( - self, - construct_id_prefix: str, - *, - index: str, - handler: str, - listener_detail_type: str, - persistent_stack: ps.PersistentStack, - data_event_bus: EventBus, -): +def _add_emailer_event_listener( + self, + config: EmailerListenerConfig, + *, + persistent_stack: ps.PersistentStack, + data_event_bus: EventBus, +):backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (2)
45-47: Fix unnecessary variable assignmentThe variable assignment before return is unnecessary and flagged by static analysis.
Apply this diff to simplify the code:
- def _generate_license_encumbrance_lifting_message(self, message_overrides=None): - """Generate a test SQS message for license encumbrance lifting events.""" - message = self._generate_license_encumbrance_message(message_overrides) - # effectiveDate is already set correctly from the base message - return message + def _generate_license_encumbrance_lifting_message(self, message_overrides=None): + """Generate a test SQS message for license encumbrance lifting events.""" + # effectiveDate is already set correctly from the base message + return self._generate_license_encumbrance_message(message_overrides)
813-833: Fix unnecessary variable assignmentSimilar to the license encumbrance lifting message, this has an unnecessary variable assignment.
Apply this diff to simplify the code:
- def _generate_privilege_encumbrance_lifting_message(self, message_overrides=None): - """Generate a test SQS message for privilege encumbrance lifting events.""" - message = self._generate_privilege_encumbrance_message(message_overrides) - # effectiveDate is already set correctly from the base message - return message + def _generate_privilege_encumbrance_lifting_message(self, message_overrides=None): + """Generate a test SQS message for privilege encumbrance lifting events.""" + # effectiveDate is already set correctly from the base message + return self._generate_privilege_encumbrance_message(message_overrides)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py(7 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/common/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(4 hunks)backend/compact-connect/lambdas/python/data-events/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(11 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/purchases/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/purchases/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(3 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt(3 hunks)backend/compact-connect/requirements-dev.txt(5 hunks)backend/compact-connect/requirements.txt(1 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(3 hunks)backend/compact-connect/tests/app/test_event_listener.py(9 hunks)backend/multi-account/control-tower/requirements-dev.txt(1 hunks)backend/multi-account/control-tower/requirements.txt(1 hunks)backend/multi-account/log-aggregation/requirements-dev.txt(1 hunks)backend/multi-account/log-aggregation/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- backend/multi-account/control-tower/requirements-dev.txt
- backend/multi-account/log-aggregation/requirements-dev.txt
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
🚧 Files skipped from review as they are similar to previous changes (19)
- backend/compact-connect/lambdas/python/common/requirements.txt
- backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt
- backend/multi-account/control-tower/requirements.txt
- backend/multi-account/log-aggregation/requirements.txt
- backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt
- backend/compact-connect/lambdas/python/data-events/requirements-dev.txt
- backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt
- backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt
- backend/compact-connect/requirements.txt
- backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt
- backend/compact-connect/lambdas/python/purchases/requirements.txt
- backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt
- backend/compact-connect/lambdas/python/common/requirements-dev.txt
- backend/compact-connect/lambdas/python/purchases/requirements-dev.txt
- backend/compact-connect/lambdas/nodejs/tests/lib/email/encumbrance-notification-service.test.ts
- backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
- backend/compact-connect/tests/app/test_event_listener.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py:220-526
Timestamp: 2025-06-18T19:52:23.159Z
Learning: The user prefers pragmatic solutions that directly address the specific problem rather than structural refactoring that doesn't provide real value. When dealing with parameter bloat warnings, they favor keyword-only arguments over dataclass wrappers to prevent positional argument misuse without over-engineering.
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
[refactor] 121-121: Too many arguments (7/5)
(R0913)
[refactor] 121-121: Too many local variables (16/15)
(R0914)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 110-110: Too many arguments (6/5)
(R0913)
[refactor] 110-110: Too many positional arguments (6/5)
(R0917)
[refactor] 137-137: Too many arguments (8/5)
(R0913)
[refactor] 137-137: Too many positional arguments (8/5)
(R0917)
[refactor] 178-178: Too many arguments (8/5)
(R0913)
[refactor] 178-178: Too many positional arguments (8/5)
(R0917)
[refactor] 219-219: Too many arguments (8/5)
(R0913)
[refactor] 219-219: Too many positional arguments (8/5)
(R0917)
[refactor] 260-260: Too many arguments (8/5)
(R0913)
[refactor] 260-260: Too many positional arguments (8/5)
(R0917)
backend/compact-connect/stacks/event_listener_stack/__init__.py
[refactor] 135-135: Too many arguments (7/5)
(R0913)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
[refactor] 27-27: Too few public methods (1/2)
(R0903)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
[refactor] 837-837: Too many local variables (16/15)
(R0914)
[error] 870-870: Too many positional arguments for function call
(E1121)
[error] 973-973: Too many positional arguments for function call
(E1121)
[error] 1046-1046: Too many positional arguments for function call
(E1121)
[error] 1096-1096: Too many positional arguments for function call
(E1121)
[error] 1146-1146: Too many positional arguments for function call
(E1121)
[error] 1232-1232: Too many positional arguments for function call
(E1121)
[error] 1312-1312: Too many positional arguments for function call
(E1121)
[error] 1386-1386: Too many positional arguments for function call
(E1121)
[error] 1465-1465: Too many positional arguments for function call
(E1121)
[error] 1517-1517: Too many positional arguments for function call
(E1121)
[error] 1559-1559: Too many positional arguments for function call
(E1121)
[error] 1567-1567: Too many positional arguments for function call
(E1121)
[error] 1663-1663: Too many positional arguments for function call
(E1121)
[error] 1731-1731: Too many positional arguments for function call
(E1121)
[error] 1804-1804: Too many positional arguments for function call
(E1121)
[error] 1854-1854: Too many positional arguments for function call
(E1121)
[error] 1904-1904: Too many positional arguments for function call
(E1121)
[error] 1990-1990: Too many positional arguments for function call
(E1121)
[error] 2070-2070: Too many positional arguments for function call
(E1121)
[error] 2144-2144: Too many positional arguments for function call
(E1121)
[error] 2223-2223: Too many positional arguments for function call
(E1121)
[error] 2275-2275: Too many positional arguments for function call
(E1121)
[error] 2320-2320: Too many positional arguments for function call
(E1121)
[error] 2328-2328: Too many positional arguments for function call
(E1121)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
[refactor] 49-49: Too few public methods (0/2)
(R0903)
🪛 Ruff (0.11.9)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
47-47: Unnecessary assignment to message before return statement
Remove unnecessary assignment
(RET504)
833-833: Unnecessary assignment to message before return statement
Remove unnecessary assignment
(RET504)
🪛 LanguageTool
backend/compact-connect/requirements-dev.txt
[duplication] ~82-~82: Possible typo: you repeated a word.
Context: ...act-connect/requirements-dev.in # pytest-cov pytest-cov==6.2.1 # via -r compact-connect/req...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (30)
backend/compact-connect/lambdas/nodejs/lib/email/encumbrance-notification-service.ts (2)
89-123: LGTM! Well-structured provider notification method.The method correctly validates recipients, fetches required configurations, constructs appropriate email content, and handles errors gracefully. The email template includes all necessary information for the provider.
39-77: Excellent error handling in state notification helper.The
getStateNotificationDatamethod properly handles missing jurisdiction configurations by logging warnings and continuing rather than failing the entire notification process. This graceful degradation approach is appropriate for notification systems.backend/compact-connect/requirements-dev.txt (1)
15-101: LGTM! Appropriate dependency version updates.The version updates for development dependencies look reasonable and maintain consistency across the project. The changes include security-focused updates (certifi, urllib3) and tool improvements (pytest, ruff, coverage).
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (2)
7-7: LGTM! Appropriate import addition for Date field.The addition of the
Datefield type to the marshmallow imports is necessary for the newEncumbranceEventDetailSchema.
49-52: Well-designed schema for encumbrance events.The
EncumbranceEventDetailSchemacorrectly extendsDataEventDetailBaseSchemaand defines the necessary fields with appropriate types and validation. The static analysis warning about "too few public methods" is a false positive - marshmallow schema classes don't require multiple public methods.backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (3)
2-3: LGTM! Appropriate type imports for enhanced type safety.Adding
dateandUUIDimports supports the improved type annotations in the method signatures.
141-145: Improved type safety with UUID parameter.Changing
provider_idfromstrtoUUIDtype and adding the structuredeffective_dateparameter enhances type safety and makes the API more explicit about expected data types.
219-299: Well-implemented privilege encumbrance event methods.The new
publish_privilege_encumbrance_eventandpublish_privilege_encumbrance_lifting_eventmethods follow the same patterns as existing license methods, ensuring consistency across the API. The parameter validation and schema usage are appropriate.backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
16-25: LGTM! Clean utility function for license type resolution.The
_get_license_type_namehelper function provides a clean interface for resolving license type names from abbreviations with appropriate error handling from the underlying utility.
27-46: Excellent error handling in provider record validation.The
_get_provider_records_and_validatefunction properly handles exceptions and provides clear error logging, which is crucial for notification processing reliability.
48-186: Well-structured notification helper functions.The three notification helper functions (
_send_provider_notification,_send_primary_state_notification,_send_additional_state_notifications) effectively encapsulate the notification logic with proper error handling and logging. These helpers address much of the code duplication that was flagged in previous reviews.
223-240: Proper event publishing with batch processing.The addition of privilege event publishing using
EventBatchWriteris well-implemented. The batch processing approach is efficient for handling multiple privilege events from a single license encumbrance.backend/compact-connect/stacks/event_listener_stack/__init__.py (1)
33-39: Excellent consolidation of event processor management!The centralized
event_processorsdictionary is a great improvement for managing multipleQueueEventListenerinstances. This makes the stack more maintainable and provides a consistent pattern for accessing event processors.backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (2)
13-32: Well-designed dataclass and protocol additions!The
EncumbranceNotificationTemplateVariablesdataclass provides excellent type safety and structure for encumbrance notification data. TheProviderNotificationMethodprotocol defines a clean interface contract.The dataclass strikes the right balance - it's not over-engineering but provides meaningful structure for the template variables that are reused across multiple methods.
244-486: Perfect implementation of keyword-only arguments approach!This implementation excellently addresses the previous concerns about parameter bloat by making all arguments keyword-only (using
*,). This prevents the error-prone positional argument usage while maintaining clear, explicit method signatures.The approach aligns perfectly with your stated preference for pragmatic solutions that directly address the specific problem without over-engineering. The consistent use of the
EncumbranceNotificationTemplateVariablesdataclass across all methods provides good structure while avoiding unnecessary complexity.backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (15)
2-4: LGTM: Additional imports for comprehensive testingThe addition of
datetime,date, andUUIDimports supports the enhanced test coverage for date handling and UUID validation in the notification templates.
10-10: LGTM: Effective date constant importAdding
DEFAULT_EFFECTIVE_DATEconstant ensures consistent date testing across encumbrance scenarios.
36-36: LGTM: Effective date inclusion in test messagesIncluding
effectiveDatein the test message structure aligns with the expected event format for encumbrance events.
53-132: LGTM: Comprehensive event publishing verificationThe test properly verifies that privilege encumbrance events are published when license encumbrance occurs. The detailed assertions for event structure, including the separation of
event_batch_writervalidation, ensure robust testing of the event publishing mechanism.
134-168: LGTM: Edge case handling verificationThe test properly verifies that no events are published when no matching privileges exist, which is important for preventing unnecessary event noise.
170-205: LGTM: Already encumbered privilege handlingThe test correctly verifies that already encumbered privileges don't trigger additional events, preventing duplicate notifications.
234-330: LGTM: Comprehensive privilege encumbrance lifting verificationThe test thoroughly validates privilege encumbrance lifting events, including proper event structure and detailed assertions. The separation of
event_batch_writervalidation follows the established pattern.
407-482: LGTM: Multiple privilege event publishing verificationThe test properly verifies that multiple privileges generate separate events and validates the event structure for each. The jurisdiction-based verification ensures events are published for all affected privileges.
1087-1100: LGTM: Proper error handling verificationThe test correctly verifies that provider retrieval failures result in batch item failures, which is essential for SQS message retry handling.
1189-1271: LGTM: Active license/privilege filteringThe test properly verifies that only active licenses and privileges generate notifications, which prevents spam to states about inactive records.
1628-1702: LGTM: License encumbrance notification testingThe comprehensive testing of license encumbrance notifications includes proper template validation and state notification verification. The jurisdiction-based notification logic is well-tested.
1845-1858: LGTM: Consistent error handling patternThe license encumbrance notification listener properly handles provider retrieval failures with the same pattern as other listeners, ensuring consistent error behavior.
2031-2112: LGTM: License encumbrance lifting notification testingThe test provides comprehensive coverage for license encumbrance lifting notifications, including proper template variable validation and state notification filtering.
2289-2389: LGTM: Edge case testing for single jurisdiction scenariosThe test properly verifies notification behavior when providers only have records in the encumbrance jurisdiction, ensuring the system handles minimal notification scenarios correctly.
835-944: Verify function call signature compatibilityThe static analysis indicates potential issues with function calls having too many positional arguments. This suggests a possible mismatch between the test code and the actual function signatures.
#!/bin/bash # Check the signature of privilege_encumbrance_listener function ast-grep --pattern 'def privilege_encumbrance_listener($$$)'LGTM: Comprehensive notification testing with proper template validation
The test thoroughly validates both provider and state notifications for privilege encumbrance, including proper template variable construction and email filtering for registered vs unregistered providers.
|
Sample emails generated:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/purchases/purchase_client.py (1)
265-265: Consider restoring the linter suppressions to avoid false positive warnings.The removal of
# noqa: RET503comments will expose RET503 warnings about missing explicit return statements. However, examining the_handle_api_errormethod (lines 237-264), it always raises exceptions and never returns normally. This means these warnings are false positives since the code paths ending withself._handle_api_error(response)calls will never reach the end without an exception.Consider either:
- Restoring the
# noqa: RET503comments to suppress these false positives- Adding explicit
return Nonestatements after the_handle_api_errorcalls for code clarity, even though they're unreachable# Option 1: Restore suppressions def void_unsettled_charge_on_credit_card( # noqa: RET503 self, order_information: dict, ) -> dict: # Option 2: Add explicit returns (though unreachable) self._handle_api_error(response) + return None # This line is never reached but satisfies linterAlso applies to: 324-324, 326-326, 490-490, 511-511
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
147-310: Comprehensive encumbrance notification implementation with good validation.The new notification cases are well-implemented with:
- Thorough validation of required template variables
- Consistent error messaging patterns
- Proper jurisdiction field validation where needed
- Appropriate use of fallback values (
event.specificEmails || []for provider notifications)Consider extracting the validation logic into helper methods to reduce repetition and improve maintainability as the switch statement grows:
private validateEncumbranceTemplateVariables( templateVariables: any, requiredFields: string[] ): void { const missingFields = requiredFields.filter(field => !templateVariables[field]); if (missingFields.length > 0) { throw new Error(`Missing required template variables: ${missingFields.join(', ')}`); } } private validateJurisdictionRequired(jurisdiction: string | undefined, templateName: string): void { if (!jurisdiction) { throw new Error(`Missing required jurisdiction field for ${templateName} template.`); } }backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
268-510: Pragmatic implementation aligned with user preferences.The implementation effectively addresses the parameter bloat concern using keyword-only arguments, which prevents positional argument misuse without over-engineering. The consistent payload construction pattern across all 8 methods makes the code maintainable and predictable.
However, consider updating the return type annotations for consistency:
- ) -> dict[str, str]: + ) -> dict[str, Any]:The actual Lambda response contains more than string values, so
dict[str, Any]would be more accurate.backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (1)
833-941: Suggest refactoring for better maintainability.This test method is comprehensive but quite long (100+ lines) with complex setup and verification logic. Consider extracting common patterns into helper methods to reduce duplication and improve readability.
The pylint warning about too many local variables (16/15) also indicates the method is doing too much.
Example refactoring approach:
def _verify_provider_notification(self, mock_provider_email, expected_template_vars): """Helper to verify provider notification calls.""" mock_provider_email.assert_called_once_with( compact=DEFAULT_COMPACT, provider_email='[email protected]', template_variables=expected_template_vars, ) def _verify_state_notifications(self, mock_state_email, expected_jurisdictions, expected_template_vars): """Helper to verify state notification calls.""" self.assertEqual(len(expected_jurisdictions), mock_state_email.call_count) calls = mock_state_email.call_args_list call_jurisdictions = [call.kwargs['jurisdiction'] for call in calls] self.assertEqual(sorted(expected_jurisdictions), sorted(call_jurisdictions)) # Additional template verification...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(5 hunks)backend/compact-connect/lambdas/nodejs/lib/email/cognito-email-service.ts(6 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(2 hunks)backend/compact-connect/lambdas/nodejs/tests/cognito-emails.test.ts(4 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(7 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(2 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(11 hunks)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py(2 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/nodejs/tests/cognito-emails.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
- backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
- backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
- backend/compact-connect/lambdas/nodejs/lib/email/cognito-email-service.ts
- backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧰 Additional context used
🧠 Learnings (1)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py:220-526
Timestamp: 2025-06-18T19:52:23.159Z
Learning: The user prefers pragmatic solutions that directly address the specific problem rather than structural refactoring that doesn't provide real value. When dealing with parameter bloat warnings, they favor keyword-only arguments over dataclass wrappers to prevent positional argument misuse without over-engineering.
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
[refactor] 833-833: Too many local variables (16/15)
(R0914)
[error] 866-866: Too many positional arguments for function call
(E1121)
[error] 969-969: Too many positional arguments for function call
(E1121)
[error] 1042-1042: Too many positional arguments for function call
(E1121)
[error] 1092-1092: Too many positional arguments for function call
(E1121)
[error] 1142-1142: Too many positional arguments for function call
(E1121)
[error] 1228-1228: Too many positional arguments for function call
(E1121)
[error] 1308-1308: Too many positional arguments for function call
(E1121)
[error] 1382-1382: Too many positional arguments for function call
(E1121)
[error] 1461-1461: Too many positional arguments for function call
(E1121)
[error] 1513-1513: Too many positional arguments for function call
(E1121)
[error] 1555-1555: Too many positional arguments for function call
(E1121)
[error] 1563-1563: Too many positional arguments for function call
(E1121)
[error] 1659-1659: Too many positional arguments for function call
(E1121)
[error] 1727-1727: Too many positional arguments for function call
(E1121)
[error] 1800-1800: Too many positional arguments for function call
(E1121)
[error] 1850-1850: Too many positional arguments for function call
(E1121)
[error] 1900-1900: Too many positional arguments for function call
(E1121)
[error] 1986-1986: Too many positional arguments for function call
(E1121)
[error] 2066-2066: Too many positional arguments for function call
(E1121)
[error] 2140-2140: Too many positional arguments for function call
(E1121)
[error] 2219-2219: Too many positional arguments for function call
(E1121)
[error] 2271-2271: Too many positional arguments for function call
(E1121)
[error] 2316-2316: Too many positional arguments for function call
(E1121)
[error] 2324-2324: Too many positional arguments for function call
(E1121)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
[refactor] 27-27: Too few public methods (1/2)
(R0903)
backend/compact-connect/lambdas/python/purchases/purchase_client.py
[refactor] 265-265: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 326-326: Too many arguments (7/5)
(R0913)
[refactor] 326-326: Too many positional arguments (7/5)
(R0917)
[refactor] 326-326: Too many local variables (28/15)
(R0914)
[refactor] 326-326: Too many branches (13/12)
(R0912)
[refactor] 326-326: Too many statements (78/50)
(R0915)
[refactor] 326-326: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🪛 Ruff (0.11.9)
backend/compact-connect/lambdas/python/purchases/purchase_client.py
324-324: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
511-511: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (8)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
11-11: Well-structured service integration.The addition of
EncumbranceNotificationServicefollows the same pattern as the existingEmailNotificationService, maintaining consistency in the codebase architecture.Also applies to: 25-25, 46-52
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
610-1176: Excellent comprehensive test coverage for encumbrance notifications.The test suites provide thorough coverage for all 8 encumbrance notification scenarios with consistent patterns:
- Success cases validating correct SES parameters and email content
- Error handling for missing template variables and jurisdictions
- Proper verification of recipient targeting and template selection
The implementation follows established testing patterns and integrates well with the existing test structure.
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
13-33: Well-structured approach for encumbrance notification data.The
EncumbranceNotificationTemplateVariablesdataclass provides excellent structure for template data, and theProviderNotificationMethodprotocol adds useful type safety for notification methods. This approach balances structure with simplicity.backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (5)
2-2: LGTM: Import additions support new test functionality.The new imports for
date,datetime,UUID, andDEFAULT_EFFECTIVE_DATEare appropriate for the expanded test coverage of encumbrance notifications.Also applies to: 4-4, 10-10
811-829: LGTM: Well-structured helper methods.The new helper methods for privilege encumbrance messages follow the established pattern and provide good abstraction for test setup.
105-130: LGTM: Thorough event publishing verification.The event publishing verification is well-structured with proper separation of EventBatchWriter validation from event data verification. This ensures comprehensive testing of the event publication functionality.
1083-1096: LGTM: Comprehensive error handling test coverage.The error handling tests properly verify that provider retrieval failures are handled gracefully and return appropriate batch item failures. This ensures robust error handling in the Lambda functions.
Also applies to: 1504-1517, 1841-1854, 2262-2275
866-866: Verify function signatures to confirm static analysis false positives.The static analysis tool reports "Too many positional arguments" for Lambda handler calls, but AWS Lambda handlers typically expect
(event, context)parameters which matches the test calls.#!/bin/bash # Description: Verify the function signatures of the encumbrance event handlers # Expected: All handlers should accept (event, context) parameters # Search for the handler function definitions ast-grep --pattern $'def $_(event, context): $$$' # Also search for any handlers with different signatures ast-grep --pattern $'def $_(event): $$$' # Search for the specific handlers mentioned in the static analysis rg -A 3 "def (license_encumbrance_listener|privilege_encumbrance_listener|license_encumbrance_notification_listener|privilege_encumbrance_lifting_listener|license_encumbrance_lifting_notification_listener)"Also applies to: 969-969, 1042-1042, 1092-1092, 1142-1142, 1228-1228, 1308-1308, 1382-1382, 1461-1461, 1513-1513, 1555-1555, 1563-1563, 1659-1659, 1727-1727, 1800-1800, 1850-1850, 1900-1900, 1986-1986, 2066-2066, 2140-2140, 2219-2219, 2271-2271, 2316-2316, 2324-2324
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/stacks/event_listener_stack/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
15-181: Excellent extraction of notification logic into reusable helper functions!The helper functions effectively address the past review comment about shared notification flow deserving a utility. They centralize error handling, logging, and provide a clean API for the notification handlers.
🧹 Nitpick comments (1)
backend/compact-connect/common_constructs/queue_event_listener.py (1)
30-48: Consider using a configuration object to manage constructor parameters.While the current parameter list is reasonable for a CDK construct, you could improve maintainability by grouping related parameters into a configuration object (e.g.,
QueueConfigandAlarmConfigdataclasses).Example structure:
@dataclass class QueueConfig: visibility_timeout: Duration = Duration.minutes(5) retention_period: Duration = Duration.hours(12) max_batching_window: Duration = Duration.seconds(15) max_receive_count: int = 3 batch_size: int = 10 @dataclass class AlarmConfig: dlq_count_alarm_threshold: int = 1 alarm_topic: ITopicThis would reduce the parameter count and make the API cleaner while maintaining flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/compact-connect/common_constructs/queue_event_listener.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(9 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(4 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(11 hunks)backend/compact-connect/stacks/event_listener_stack/__init__.py(4 hunks)backend/compact-connect/stacks/notification_stack.py(7 hunks)backend/compact-connect/tests/app/test_event_listener.py(8 hunks)backend/compact-connect/tests/app/test_notification_stack.py(1 hunks)backend/compact-connect/tests/common_constructs/test_queue_event_listener.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/tests/app/test_event_listener.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/stacks/event_listener_stack/init.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/common_constructs/queue_event_listener.py
[refactor] 30-30: Too many arguments (14/5)
(R0913)
[refactor] 16-16: Too few public methods (0/2)
(R0903)
backend/compact-connect/stacks/notification_stack.py
[refactor] 149-149: Too many arguments (7/5)
(R0913)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
[refactor] 120-120: Too many arguments (7/5)
(R0913)
[refactor] 120-120: Too many local variables (16/15)
(R0914)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
[refactor] 833-833: Too many local variables (16/15)
(R0914)
[error] 866-866: Too many positional arguments for function call
(E1121)
[error] 969-969: Too many positional arguments for function call
(E1121)
[error] 1042-1042: Too many positional arguments for function call
(E1121)
[error] 1092-1092: Too many positional arguments for function call
(E1121)
[error] 1142-1142: Too many positional arguments for function call
(E1121)
[error] 1228-1228: Too many positional arguments for function call
(E1121)
[error] 1308-1308: Too many positional arguments for function call
(E1121)
[error] 1382-1382: Too many positional arguments for function call
(E1121)
[error] 1461-1461: Too many positional arguments for function call
(E1121)
[error] 1513-1513: Too many positional arguments for function call
(E1121)
[error] 1558-1558: Too many positional arguments for function call
(E1121)
[error] 1566-1566: Too many positional arguments for function call
(E1121)
[error] 1662-1662: Too many positional arguments for function call
(E1121)
[error] 1730-1730: Too many positional arguments for function call
(E1121)
[error] 1803-1803: Too many positional arguments for function call
(E1121)
[error] 1853-1853: Too many positional arguments for function call
(E1121)
[error] 1903-1903: Too many positional arguments for function call
(E1121)
[error] 1989-1989: Too many positional arguments for function call
(E1121)
[error] 2069-2069: Too many positional arguments for function call
(E1121)
[error] 2143-2143: Too many positional arguments for function call
(E1121)
[error] 2222-2222: Too many positional arguments for function call
(E1121)
[error] 2274-2274: Too many positional arguments for function call
(E1121)
[error] 2319-2319: Too many positional arguments for function call
(E1121)
[error] 2327-2327: Too many positional arguments for function call
(E1121)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (14)
backend/compact-connect/tests/common_constructs/test_queue_event_listener.py (1)
1-290: Excellent test coverage for QueueEventListener!The test suite comprehensively covers all aspects of the QueueEventListener construct including default parameters, custom parameters, exposed attributes, alarm counts, and construct ID propagation. The tests are well-structured and properly verify CloudFormation resource creation.
backend/compact-connect/common_constructs/queue_event_listener.py (1)
16-28: Good architectural improvements for flexibility and modularity.The changes to accept explicit parameters instead of persistent_stack improve the construct's reusability and make dependencies more explicit. The addition of default constants provides sensible defaults.
backend/compact-connect/tests/app/test_notification_stack.py (1)
1-567: Comprehensive test coverage for NotificationStack event processors!The test suite thoroughly validates all notification event processors including privilege purchase, license encumbrance, and privilege encumbrance handlers. The consistent test pattern and proper handling of dynamic fields ensure reliable test execution.
backend/compact-connect/stacks/notification_stack.py (2)
41-46: Well-structured addition of encumbrance notification listeners.The introduction of the
event_processorsdictionary and systematic addition of notification listeners provides a clean, extensible pattern for managing event-driven notifications.
149-208: Excellent refactoring with the helper method to reduce code duplication.The
_add_emailer_event_listenermethod effectively consolidates the common pattern for creating notification event listeners, making the code more maintainable and consistent.backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (3)
183-237: Good separation of concerns with event publishing.The updated
license_encumbrance_listenernow properly publishes privilege encumbrance events for each affected privilege, enabling fine-grained notification handling downstream.
298-371: Well-implemented notification handler following established patterns.The
privilege_encumbrance_notification_listenerproperly uses the helper functions and follows the "maximally noisy" notification approach as discussed in past reviews.
120-130: I’d like to inspect the definition of_send_primary_state_notificationto see ifprovider_idis actually referenced in its body.#!/bin/bash rg -n "def _send_primary_state_notification" -A 40 backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (6)
2-2: LGTM: Import additions support new test functionality.The new imports for
date,datetime,UUID, andDEFAULT_EFFECTIVE_DATEare appropriately added to support the expanded test coverage for encumbrance notifications with date handling and UUID validation.Also applies to: 4-4, 10-10
36-36: LGTM: Effective date integration aligns with PR objectives.Adding
DEFAULT_EFFECTIVE_DATEto the test message properly supports the new effective date functionality mentioned in the PR objectives.
811-830: LGTM: Privilege message generation follows established patterns.The new privilege encumbrance message generation methods appropriately mirror the existing license message generation pattern, ensuring consistency in test data setup.
866-866: Static analysis false positives can be safely ignored.The pylint "Too many positional arguments" errors are false positives. All flagged function calls follow the standard AWS Lambda handler pattern
handler_function(event, context)with exactly two arguments, which is correct. The static analysis tool appears to be misunderstanding the function signatures, possibly due to decorators or dynamic definitions not visible in this context.Also applies to: 969-969, 1042-1042, 1092-1092, 1142-1142, 1228-1228, 1308-1308, 1382-1382, 1461-1461, 1513-1513, 1558-1558, 1566-1566, 1662-1662, 1730-1730, 1803-1803, 1853-1853, 1903-1903, 1989-1989, 2069-2069, 2143-2143, 2222-2222, 2274-2274, 2319-2319, 2327-2327
833-833: Acceptable complexity for comprehensive test method.The "Too many local variables" warning is acceptable for this comprehensive test method that needs extensive setup for testing multiple notification scenarios with different jurisdictions and provider states.
831-2389: Excellent comprehensive test coverage for encumbrance notification system.The test suite demonstrates excellent coverage of the encumbrance notification functionality:
Strengths:
- Comprehensive scenarios: Tests cover both license and privilege encumbrance/lifting for registered/unregistered providers
- Edge case handling: Includes tests for provider retrieval failures, mixed jurisdictions, and inactive records
- Proper separation: Clear distinction between data manipulation tests and notification tests
- Thorough assertions: Validates both email service calls and template variable content
- Good mocking practices: Appropriate use of mocks to isolate notification logic
Coverage highlights:
- Multiple jurisdiction scenarios
- Active/inactive license and privilege handling
- Provider registration status variations
- Error handling and batch failure scenarios
- Template variable validation with UUIDs and dates
This test suite provides strong confidence in the encumbrance notification system's reliability and correctness.
|
@jlkravitz This is ready for your review. Thanks |
backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Outdated
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/purchase_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
298-370: Handler follows established patterns but contributes to duplicationThe
privilege_encumbrance_notification_listeneris correctly implemented and follows the established notification pattern. However, it's nearly identical to the other notification handlers, contributing to the code duplication issue mentioned in previous reviews.The logic for notifications is sound:
- Validates event schema
- Retrieves provider records
- Sends provider notification if registered
- Sends primary state notification
- Sends additional state notifications
373-445: Consistent implementation with other notification handlersThe
privilege_encumbrance_lifting_notification_listenercorrectly mirrors the encumbrance notification logic with appropriate method calls for lifting events.
448-520: License notification handler correctly separates concernsThe
license_encumbrance_notification_listenerappropriately handles only notifications without performing data operations, maintaining clear separation between data mutation and notification concerns.
523-595: Complete notification handler implementationThe
license_encumbrance_lifting_notification_listenercompletes the notification handler set with consistent implementation patterns.
120-180: Significant code duplication across notification handlersWhile this function works correctly, the notification handlers that follow contain nearly identical logic (~120 lines each). This relates to a previous review comment suggesting extraction of shared notification flow logic.
Consider refactoring to reduce duplication by extracting a common notification orchestration function that accepts:
- Event details
- Notification method callbacks (provider/state)
- Notification type string
This would eliminate the pylint warnings about too many locals and improve maintainability.
Example refactor approach:
+def _handle_encumbrance_notifications( + detail: dict, + provider_notification_method: ProviderNotificationMethod, + state_notification_method: ProviderNotificationMethod, + notification_type: str, +) -> None: + """Common notification orchestration logic.""" + # Extract common logic from the notification handlers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(4 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (5)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:116-201
Timestamp: 2025-05-30T13:48:25.375Z
Learning: In encumbrance handling code, prefer to keep privilege and license encumbrance lifting implementations decoupled rather than extracting shared logic, as requirements between these implementations are likely to change in the future.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:53-60
Timestamp: 2025-04-29T02:10:38.400Z
Learning: Lambda functions in the provider-data-v1 module that are exposed via API Gateway endpoints (like encumbrance_handler) should only be called through the API, not directly. Therefore, they don't need to handle various body formats as the API Gateway will consistently provide event['body'] as a JSON string.
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/components/LicenseCard/LicenseCard.ts:414-443
Timestamp: 2025-06-24T00:02:39.944Z
Learning: In LicenseCard component's clickUnencumberItem method (webroot/src/components/LicenseCard/LicenseCard.ts), complex event handling for checkbox interactions is intentionally designed to ensure consistent behavior across checkbox input, wrapper label, and outer selection parent elements for custom UI requirements. This complexity should be preserved rather than simplified.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.772Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (8)
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #824
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/encumbrance.py:116-201
Timestamp: 2025-05-30T13:48:25.375Z
Learning: In encumbrance handling code, prefer to keep privilege and license encumbrance lifting implementations decoupled rather than extracting shared logic, as requirements between these implementations are likely to change in the future.
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #798
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/init.py:122-129
Timestamp: 2025-05-13T20:58:28.713Z
Learning: In the CompactConnect codebase, required fields should use direct dictionary access (e.g., self._data['field']) to intentionally raise KeyError if missing, while optional fields should use .get() with defaults. This is by design to ensure data integrity issues are surfaced immediately rather than masked with default values.
</retrieved_learning>
<retrieved_learning>
Learnt from: jusdino
PR: #864
File: backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py:220-526
Timestamp: 2025-06-18T19:52:23.159Z
Learning: The user prefers pragmatic solutions that directly address the specific problem rather than structural refactoring that doesn't provide real value. When dealing with parameter bloat warnings, they favor keyword-only arguments over dataclass wrappers to prevent positional argument misuse without over-engineering.
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, cc_common.config._Config.current_standard_datetime is a property (not a method), and can be properly patched in tests by directly supplying a datetime value rather than wrapping it in a lambda: @patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP)).
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, cc_common.config._Config.current_standard_datetime is a property rather than a method, and should be patched in tests by directly providing a datetime value: @patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP)).
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:38-40
Timestamp: 2025-04-29T21:14:36.205Z
Learning: In the CompactConnect codebase, cc_common.config._Config.current_standard_datetime is implemented as a property that returns datetime.now(tz=UTC).replace(microsecond=0). The correct way to patch it in tests is by directly providing the datetime value, not wrapping it in a lambda: @patch('cc_common.config._Config.current_standard_datetime', datetime.fromisoformat(DEFAULT_DATE_OF_UPDATE_TIMESTAMP)).
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
</retrieved_learning>
🧬 Code Graph Analysis (1)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
EncumbranceNotificationTemplateVariables(14-24)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (4)
privilege_encumbrance_notification_listener(299-370)privilege_encumbrance_lifting_notification_listener(374-445)license_encumbrance_notification_listener(449-520)license_encumbrance_lifting_notification_listener(524-595)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(379-393)put_default_privilege_record_in_provider_table(303-309)put_default_license_record_in_provider_table(237-243)
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
[refactor] 120-120: Too many arguments (7/5)
(R0913)
[refactor] 120-120: Too many local variables (16/15)
(R0914)
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
[error] 866-866: Too many positional arguments for function call
(E1121)
[error] 973-973: Too many positional arguments for function call
(E1121)
[error] 1048-1048: Too many positional arguments for function call
(E1121)
[error] 1099-1099: Too many positional arguments for function call
(E1121)
[error] 1149-1149: Too many positional arguments for function call
(E1121)
[error] 1236-1236: Too many positional arguments for function call
(E1121)
[error] 1317-1317: Too many positional arguments for function call
(E1121)
[error] 1390-1390: Too many positional arguments for function call
(E1121)
[error] 1469-1469: Too many positional arguments for function call
(E1121)
[error] 1520-1520: Too many positional arguments for function call
(E1121)
[error] 1565-1565: Too many positional arguments for function call
(E1121)
[error] 1573-1573: Too many positional arguments for function call
(E1121)
[error] 1669-1669: Too many positional arguments for function call
(E1121)
[error] 1738-1738: Too many positional arguments for function call
(E1121)
[error] 1813-1813: Too many positional arguments for function call
(E1121)
[error] 1864-1864: Too many positional arguments for function call
(E1121)
[error] 1914-1914: Too many positional arguments for function call
(E1121)
[error] 2001-2001: Too many positional arguments for function call
(E1121)
[error] 2082-2082: Too many positional arguments for function call
(E1121)
[error] 2155-2155: Too many positional arguments for function call
(E1121)
[error] 2234-2234: Too many positional arguments for function call
(E1121)
[error] 2285-2285: Too many positional arguments for function call
(E1121)
[error] 2330-2330: Too many positional arguments for function call
(E1121)
[error] 2338-2338: Too many positional arguments for function call
(E1121)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (10)
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (6)
15-23: LGTM - Helper function is well-designedThe function provides a clean interface for license type name lookup with appropriate error handling from the underlying utility.
26-44: LGTM - Good error handling and separation of concernsThe function appropriately handles provider record retrieval with comprehensive error logging and re-raises exceptions for proper failure handling.
47-81: LGTM - Conditional notification logic is well-implementedThe function correctly checks for registered email addresses before sending notifications and handles the optional nature of provider registration appropriately.
84-117: LGTM - Clean state notification implementationThe function provides consistent error handling and logging for state notifications.
217-235: LGTM - Event publishing logic is correctThe privilege event publishing implementation correctly:
- Uses batch writing for efficient event publishing
- Publishes events for each affected privilege
- Includes proper logging and context
- Passes the privilege jurisdiction (not license jurisdiction) as required
The decision to send notifications for automatic privilege encumbrances aligns with the updated requirements mentioned in past review discussions.
274-291: LGTM - Consistent event publishing for lifting eventsThe privilege encumbrance lifting event publishing follows the same pattern as the encumbrance events, maintaining consistency in the event-driven architecture.
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py (4)
831-940: Excellent comprehensive test coverage for privilege encumbrance notifications.The test structure for privilege encumbrance notification handling is well-designed and covers all the essential scenarios:
- Registered vs unregistered providers
- Proper jurisdiction identification for notifications
- Provider retrieval failure handling
- Template variable validation with
EncumbranceNotificationTemplateVariablesThe assertions properly validate both provider and state notifications with appropriate template variables. Using comprehensive object assertions (as preferred per past feedback) makes the tests clear and maintainable.
949-951: Good documentation of edge case testing rationale.The comment clearly explains that unregistered providers shouldn't have privileges in normal operation, but this test stress-tests the notification logic's resilience. This addresses the earlier review feedback about documenting the purpose of this edge case scenario.
1634-2400: Comprehensive test coverage for license encumbrance notifications.The license encumbrance notification tests mirror the privilege tests with excellent coverage of:
- Provider registration status handling
- Multi-jurisdiction notification logic
- Inactive license/privilege inclusion
- Duplicate jurisdiction exclusion
- Edge cases with no additional jurisdictions
The test structure is consistent and thorough, ensuring the notification system works reliably across all scenarios.
2-4: Clean import additions support new notification functionality.The added imports (
date,datetime,UUID,DEFAULT_EFFECTIVE_DATE) are appropriately scoped for the new notification testing requirements. These support theEncumbranceNotificationTemplateVariablesusage and date handling in the comprehensive test suite.Also applies to: 10-10, 36-36
backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
Show resolved
Hide resolved
Description List
Testing List
Closes Adverse Action - notify the provider and all states where a provider holds a privilege or license #675
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores