Allow compact admins to audit military records #1233
Conversation
📝 WalkthroughWalkthroughAdds military audit flows: provider-level Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant API as MilitaryAuditHandler
participant DC as DataClient
participant DB as DynamoDB
participant EventBus as EventBusClient
participant Email as EmailServiceClient
Admin->>API: PATCH /v1/compacts/{c}/providers/{id}/militaryAudit (status, note)
API->>API: validate request
API->>DC: process_military_audit(compact, provider_id, status, note)
DC->>DB: Begin transaction
DC->>DB: Update provider (militaryStatus, militaryStatusNote)
DC->>DB: Insert provider_update (MILITARY_AUDIT)
DC->>DB: Commit transaction
API->>EventBus: publish_military_audit_event(detail)
EventBus->>Email: route to military_audit listener
Email->>Email: send approved/declined notification to provider
API-->>Admin: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2026-01-14T16:55:31.926ZApplied to files:
📚 Learning: 2025-12-16T21:43:07.408ZApplied to files:
📚 Learning: 2025-08-22T21:04:46.552ZApplied to files:
📚 Learning: 2025-10-24T21:40:48.912ZApplied to files:
📚 Learning: 2026-01-07T21:20:12.524ZApplied to files:
📚 Learning: 2025-07-15T02:02:56.052ZApplied to files:
📚 Learning: 2025-04-29T02:10:38.400ZApplied to files:
📚 Learning: 2025-07-21T20:39:47.625ZApplied to files:
📚 Learning: 2025-06-17T19:05:36.255ZApplied to files:
📚 Learning: 2025-09-03T22:16:52.533ZApplied to files:
📚 Learning: 2025-08-22T15:49:43.755ZApplied to files:
📚 Learning: 2026-01-05T22:50:09.696ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
81-84: Consider adding length validation tomilitaryStatusNotefor consistency.The API model (
patch_military_audit_request_model) enforcesmax_length=5000formilitaryStatusNote, but the schema field here has no length constraint. While the API gateway will validate incoming requests, addingvalidate=Length(max=5000)to the schema would provide defense-in-depth and document the constraint at the data layer.🔎 Proposed fix
# Military audit status fields militaryStatus = MilitaryAuditStatusField(required=False, allow_none=False) - militaryStatusNote = String(required=False, allow_none=False) + militaryStatusNote = String(required=False, allow_none=False, validate=Length(max=5000))backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py (1)
185-223: Consider adding a test for status transition scenarios.The test for provider update record creation is thorough and verifies:
- Correct
updateType(MILITARY_AUDIT)- Previous state capture (
tentative→approved)- Updated values including the note
However, consider adding a test for transitioning between
approvedanddeclined(or vice versa) to ensure the audit trail correctly captures re-audits.backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (3)
10-11: Remove incorrectnoqacomment.The
# noqa: ARG001comment indicates an unused argument, buttrackeris actually used throughout the function (lines 60, 85, 93). This comment appears to be incorrect or stale.🔎 Proposed fix
@sqs_handler_with_notification_tracking -def military_audit_notification_listener(message: dict, tracker: NotificationTracker): # noqa: ARG001 +def military_audit_notification_listener(message: dict, tracker: NotificationTracker): """
42-50: Simplify UUID conversion and consider recording failure for provider lookup errors.Two observations:
provider_idis already a UUID from the schema (perMilitaryAuditEventDetailSchema), soUUID(str(provider_id))is unnecessary.When provider record retrieval fails, the exception is re-raised but the failure is not recorded to the tracker. This differs from the notification sending block (lines 91-100) which records failures before re-raising. Consider recording the failure for consistency and observability.
🔎 Proposed fix
# Get provider records to find registered email try: provider_records = config.data_client.get_provider_user_records( compact=compact, - provider_id=UUID(str(provider_id)), + provider_id=provider_id, ) provider_record = provider_records.get_provider_record() except Exception as e: logger.error('Failed to retrieve provider records for notification', exception=str(e)) + # Determine event type for failure tracking + event_type = ( + EventType.MILITARY_AUDIT_APPROVED if audit_result == 'approved' else EventType.MILITARY_AUDIT_DECLINED + ) + tracker.record_failure( + recipient_type=RecipientType.PROVIDER, + provider_id=provider_id, + event_type=event_type, + event_time=event_time, + error_message=f'Failed to retrieve provider records: {e}', + ) raise
59-67: Handle potentialNonetracker for edge cases.The
sqs_handler_with_notification_trackingdecorator passesNoneas the tracker when no compact is found in the message (per the decorator code at lines 498-501 inutils.py). While this shouldn't happen with properly formed military audit events, adding a guard would make the code more defensive.🔎 Proposed fix
# Check if we should send the notification (idempotency) - if not tracker.should_send_provider_notification(): + if tracker and not tracker.should_send_provider_notification(): logger.info('Skipping provider notification (already sent successfully)') returnAnd similarly for
tracker.record_successandtracker.record_failurecalls.backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
59-78: Consider resilience for the event publishing step.If
process_military_auditsucceeds butpublish_military_audit_eventfails, the audit status is recorded in the database but the licensee notification event won't be published. This could result in providers not receiving approval/decline notifications.Consider whether this is acceptable or if the notification should be retried, logged for manual follow-up, or if both operations should be atomic.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
741-746: Potential edge case: multiple records could be set to ACTIVE.If multiple military affiliation records have identical
dateOfUploadtimestamps (theoretically possible in concurrent scenarios), this comparison would set all of them toACTIVEinstead of just one.Consider using object identity or adding a secondary sort key (like
skor a unique identifier) to ensure deterministic selection:🔎 Proposed fix
for record in initializing_military_affiliation_records: - if record.dateOfUpload == latest_military_affiliation_record.dateOfUpload: + # Use object identity to ensure only the exact record selected as latest is set to active + if record is latest_military_affiliation_record: record.update({'status': MilitaryAffiliationStatus.ACTIVE.value}) else: record.update({'status': MilitaryAffiliationStatus.INACTIVE.value})
903-921: Consider adding error handling for the transaction.Other similar methods in this file (e.g.,
complete_military_affiliation_initialization,create_provider_privileges) wraptransact_write_itemscalls in try/except blocks to handleClientErrorand provide more descriptive error messages. This transaction lacks such handling.🔎 Proposed fix
# Execute both updates in a transaction - self.config.dynamodb_client.transact_write_items( - TransactItems=[ - # Update provider record - { - 'Put': { - 'TableName': self.config.provider_table_name, - 'Item': TypeSerializer().serialize(provider_record.serialize_to_database_record())['M'], - } - }, - # Create provider update record - { - 'Put': { - 'TableName': self.config.provider_table_name, - 'Item': TypeSerializer().serialize(provider_update_record.serialize_to_database_record())['M'], - } - }, - ] - ) + try: + self.config.dynamodb_client.transact_write_items( + TransactItems=[ + # Update provider record + { + 'Put': { + 'TableName': self.config.provider_table_name, + 'Item': TypeSerializer().serialize(provider_record.serialize_to_database_record())['M'], + } + }, + # Create provider update record + { + 'Put': { + 'TableName': self.config.provider_table_name, + 'Item': TypeSerializer().serialize(provider_update_record.serialize_to_database_record())['M'], + } + }, + ] + ) + except ClientError as e: + logger.error('Failed to process military audit transaction', error=str(e)) + raise CCAwsServiceException('Failed to process military audit') from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/common/cc_common/event_bus_client.pybackend/compact-connect/lambdas/python/common/cc_common/event_state_client.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/stacks/api_lambda_stack/provider_management.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/stacks/api_stack/v1_api/provider_management.pybackend/compact-connect/stacks/notification_stack.py
🧰 Additional context used
🧠 Learnings (18)
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/common/cc_common/event_bus_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/stacks/notification_stack.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.pybackend/compact-connect/stacks/api_stack/v1_api/provider_management.pybackend/compact-connect/lambdas/python/common/cc_common/event_bus_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/event_state_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/stacks/api_lambda_stack/provider_management.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py
📚 Learning: 2025-04-29T15:30:08.849Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py:17-23
Timestamp: 2025-04-29T15:30:08.849Z
Learning: The MilitaryAffiliationData class in CompactConnect is explicitly designed with read-only properties as stated in its docstring, even though it inherits from CCDataClass which supports initialization without data followed by using setters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py
📚 Learning: 2025-05-13T21:20:51.133Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 798
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py:86-90
Timestamp: 2025-05-13T21:20:51.133Z
Learning: The implementation of `PositiveDecimal` in `cc_common/data_model/schema/fields.py` is intentionally kept simple without support for stacked validators. If more complex validation is needed, the preferred approach is to create another custom field class.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In the CompactConnect backend, provider data sanitization follows a centralized architecture: the ProviderRecordUtility.generate_api_response_object() method serves as the single source for constructing provider response objects and includes sanitization of sensitive profile fields. Downstream functions like sanitize_provider_data_based_on_caller_scopes() in utils.py receive already-sanitized data, making additional sanitization calls redundant. This ensures sensitive fields are removed at the source rather than requiring multiple sanitization points throughout the codebase.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: The CompactConnect provider data sanitization architecture follows this flow: endpoints call get_provider_information() → which calls provider_user_records.generate_api_response_object() → which includes sanitization of sensitive profile fields (emailVerificationExpiry, emailVerificationCode, pendingEmailAddress, recoveryToken, recoveryExpiry) → sanitized data then flows to sanitize_provider_data_based_on_caller_scopes() for additional scope-based filtering. This centralized approach ensures sensitive fields are removed at the source rather than requiring sanitization at multiple return points.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py
📚 Learning: 2025-08-21T20:24:39.042Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:0-0
Timestamp: 2025-08-21T20:24:39.042Z
Learning: When validating identifier fields like providerId in Marshmallow schemas, prefer using the UUID field type over String with Length validation, as UUID provides automatic format validation and is more semantically appropriate.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-10-15T19:56:58.050Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.050Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py
🧬 Code graph analysis (15)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (10)
compact(29-30)compact(194-195)providerId(25-26)providerId(190-191)militaryStatus(151-158)militaryStatusNote(161-167)ProviderUpdateData(170-211)updateType(186-187)previous(202-203)updatedValues(206-207)backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
military_audit_handler(16-78)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
UpdateCategory(305-323)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
pop(77-78)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (1)
required(17-18)
backend/compact-connect/stacks/notification_stack.py (2)
backend/compact-connect/stacks/persistent_stack/event_bus.py (1)
EventBus(9-25)backend/compact-connect/stacks/event_state_stack/__init__.py (1)
EventStateStack(9-36)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
MilitaryAuditEventDetailSchema(89-96)backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (6)
EventType(23-31)NotificationTracker(117-239)RecipientType(9-13)should_send_provider_notification(134-141)record_success(153-194)record_failure(196-239)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInternalException(44-45)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
sqs_handler_with_notification_tracking(471-530)get(86-87)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_user_records(178-221)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
compactConnectRegisteredEmailAddress(77-83)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (2)
send_military_audit_approved_notification(764-784)send_military_audit_declined_notification(786-810)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (4)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
military_audit_handler(16-78)backend/compact-connect/common_constructs/cc_api.py (2)
parameter_body_validator(240-241)message_response_model(254-264)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
patch_military_audit_request_model(2887-2911)message_response_model(21-39)backend/compact-connect/stacks/state_api_stack/v1_api/api_model.py (1)
message_response_model(21-39)
backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
MilitaryAuditEventDetailSchema(89-96)backend/compact-connect/lambdas/python/common/cc_common/event_batch_writer.py (1)
EventBatchWriter(4-49)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(261-265)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (2)
MilitaryAffiliationData(10-51)dateOfUpload(46-47)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
MilitaryAuditStatusField(136-138)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compact(29-30)compact(194-195)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
compact(32-33)compact(172-173)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/investigation/__init__.py (2)
compact(71-72)compact(75-76)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
compact(28-29)compact(112-113)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (2)
compact(58-59)compact(108-109)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
MilitaryAuditStatus(296-302)UpdateCategory(305-323)from_database_record(124-139)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_s3_events.py (1)
process_provider_s3_events(7-40)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_top_level_record(1010-1033)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (10)
compact(29-30)compact(194-195)militaryStatus(151-158)militaryStatusNote(161-167)ProviderUpdateData(170-211)updateType(186-187)providerId(25-26)providerId(190-191)previous(202-203)updatedValues(206-207)
backend/compact-connect/stacks/api_lambda_stack/provider_management.py (3)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
military_audit_handler(16-78)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(19-138)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
provider_table(88-89)
🪛 GitHub Actions: Check-CompactConnect
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
[error] 704-704: No provider record found for user.
🔇 Additional comments (39)
backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json (1)
6-7: LGTM! Default military audit fields added correctly.The addition of
militaryStatus: "notApplicable"andmilitaryStatusNote: ""aligns with the expected default behavior for providers without military documentation.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (1)
457-458: LGTM! Correct filtering of military status fields from public endpoint.The test correctly ensures that
militaryStatusandmilitaryStatusNoteare excluded from public provider responses, maintaining appropriate privacy controls.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (2)
15-15: LGTM! Import added for military audit status enum.
136-139: LGTM! Field validator follows established pattern.The
MilitaryAuditStatusFieldimplementation is consistent with other status field validators in this file.backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (2)
14-14: LGTM! Import added for military audit event schema.
550-587: LGTM! Event publishing method follows established pattern.The
publish_military_audit_eventmethod is well-implemented and consistent with other event publishing methods in this class. The conditional inclusion ofauditNote(lines 576-577) correctly handles the optional parameter.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py (1)
376-377: LGTM! Correct filtering of military status note for general scope.The test correctly removes only
militaryStatusNotewhen the caller lacks read-private scope, while keepingmilitaryStatusvisible. This access control design makes sense: the status is general information, but the note may contain sensitive details requiring elevated permissions.backend/compact-connect/stacks/notification_stack.py (2)
70-72: LGTM! Military audit listener properly registered.
350-362: LGTM! Notification listener follows established pattern.The
_add_military_audit_notification_listenermethod is correctly implemented following the same pattern as other notification listeners in this stack. The detail type'militaryAffiliation.audit'correctly matches the event type published by the event bus client.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (2)
2-2: LGTM! Schema import added for request validation.
27-32: LGTM! Request schema correctly validates audit actions.The
MilitaryAuditRequestSchemaappropriately restrictsmilitaryStatusto only'approved'or'declined', preventing system-managed statuses like'notApplicable'or'tentative'from being set via this endpoint. The use ofSchema(notForgivingSchema) provides strict validation suitable for PATCH requests.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
150-167: LGTM! Properties follow established pattern with clear documentation.The new
militaryStatusandmilitaryStatusNoteproperties are well-implemented with informative docstrings explaining their purpose and expected values.backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
2886-2911: PATCH military audit request model aligns with handler and event schemaThe new
patch_military_audit_request_modelcleanly matches the handler contract:militaryStatusis constrained to approved/declined andmilitaryStatusNoteis optional with a reasonable length cap, andadditional_properties=Falsekeeps the payload tight. The lazy init and naming follow the existing pattern for other PATCH models.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
296-312: Military audit status and update categories are well‑scoped and backward compatible
MilitaryAuditStatuscleanly models the provider‑level audit lifecycle, and the newUpdateCategorymembers (MILITARY_AUDIT,MILITARY_FILE_UPLOAD) integrate with existing patterns without impacting prior categories. The string values are consistent with what handlers and tests expect.backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py (1)
24-31: EventType extensions cleanly cover military audit notificationsThe new
MILITARY_AUDIT_APPROVEDandMILITARY_AUDIT_DECLINEDevent types fit the existing naming convention and plug intorecord_notification_attemptwithout changing behavior for existing events.backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
60-61: Military audit Lambda wiring is consistent with existing provider handlersThe new
MilitaryAuditHandleris initialized, logged, and permissioned in line with the investigation/encumbrance handlers: same environment block, provider/staff table access, and event‑busPutEventsgrant, plus the standard IAM nag suppression. This should be sufficient for the handler logic shown inhandlers/military_audit.pywithout over‑privileging it.Also applies to: 375-406
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (1)
2-232: New military affiliation S3 tests robustly cover status transitions and update historySeeding the provider with a declined status/note, then asserting:
- S3 upload flips the newest affiliation to
activeand older toinactive,- provider.militaryStatus/militaryStatusNote transition to
tentative/'', and- exactly one
MILITARY_FILE_UPLOADProviderUpdate capturing previous vs updated military fields,gives strong end‑to‑end coverage of the new military‑file‑upload semantics. The use of ISO timestamps for
dateOfUploadand explicit documentKeys makes the “latest record wins” logic unambiguous.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (1)
16-20: Provider API schemas expose military audit state with appropriate access levelsWiring
militaryStatusthrough both general and read‑private schemas, while limitingmilitaryStatusNoteto read‑private only and keeping public schemas unchanged, matches the intended visibility model. UsingMilitaryAuditStatusFieldkeeps validation consistent with the core enum.Also applies to: 100-103, 157-159
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
8-9: Military audit event detail schema matches the new event payload shape
MilitaryAuditEventDetailSchemacaptures exactly the fields used by the military audit events (compact, providerId, auditResult/Note, eventTime), withauditResultconstrained to the two terminal outcomes. Keeping it separate fromDataEventDetailBaseSchema(no jurisdiction) reflects the compact‑level nature of the audit.Also applies to: 89-96
backend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py (1)
1-111: Military audit notification tests give solid coverage of listener behaviorThe new tests correctly drive
military_audit_notification_listenerthrough SQS‑wrapped EventBridge payloads, asserting:
- approved/declined flows call the right email methods with expected kwargs and yield no batch failures, and
- email send errors result in a single
batchItemFailuresentry.This provides good confidence in both notification wiring and failure handling for the military audit events.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
25-25: LGTM!The import of
MilitaryAuditStatusFieldis correctly added alongside other field imports.
214-215: LGTM!The military status fields are correctly mirrored in
ProviderUpdatePreviousRecordSchema, enabling proper tracking of previous state in update records. This is consistent with the existing pattern for other snapshot fields.backend/compact-connect/stacks/api_stack/v1_api/provider_management.py (2)
102-105: LGTM!The military audit endpoint is correctly wired using
admin_method_options, ensuring only compact administrators can access this functionality. This aligns with the PR objective of enabling compact directors to audit military documents.
458-481: LGTM!The
_add_military_auditmethod follows the established patterns in this file:
- Creates a sub-resource under
provider_resource- Uses PATCH method appropriate for partial updates
- Includes request validation, proper authorization, and consistent timeout
- Returns a message response model matching other similar endpoints
The implementation is clean and consistent with existing admin endpoints like encumbrance and investigation.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py (4)
16-46: LGTM!The test class setup is well-structured:
- Uses
@mock_awsfor AWS service mocking- Patches
current_standard_datetimefor deterministic time-based tests- The
_when_testing_military_audithelper method cleanly encapsulates test setup and supports both default and custom provider scenarios
48-104: LGTM!Good coverage of the core happy paths:
- Approval returns OK
- Decline with note returns OK
- Provider record is updated with correct status
- Declined status correctly captures the note
The assertions verify both the response and the side effects on the database records.
106-158: LGTM!Excellent error case coverage:
- Invalid status value returns 400 with descriptive message
- Missing required status field returns 400
- Missing military affiliation returns 404
These tests ensure proper input validation and error handling.
160-183: LGTM!Authorization test correctly verifies that non-admin users (with
readGeneralscope) receive a 403 Forbidden response. This is critical for ensuring the endpoint is properly secured for admin-only access.backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (1)
69-100: LGTM!The notification sending logic is well-structured:
- Correctly determines event type based on audit result
- Sends appropriate notification (approved vs declined with note)
- Records success/failure to the tracker for idempotency tracking
- Properly re-raises exceptions after recording failure
This follows the established pattern for notification handlers in the codebase.
backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (2)
764-784: LGTM!The
send_military_audit_approved_notificationmethod follows the established pattern:
- Uses keyword-only arguments for clarity
- Correctly specifies the template name and recipient type
- Empty
templateVariablesis appropriate since approved notifications don't need additional context
786-810: LGTM!The
send_military_audit_declined_notificationmethod is well-implemented:
- Includes the
auditNotein template variables to explain the decline reason to the provider- Follows the same pattern as the approved notification
- Keyword-only arguments ensure clear call sites
Both new methods integrate cleanly with the existing email service infrastructure.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
18-18: LGTM!Import addition for
MilitaryAuditStatusis correctly placed with the related imports from the same module.
799-807: LGTM!The method correctly returns the most recent military affiliation record using consistent sorting logic.
958-962: LGTM!Setting defaults ensures backward compatibility for existing provider records that don't have military audit status fields, correctly defaulting to
NOT_APPLICABLEstatus.backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (2)
1-11: LGTM!Imports are correctly organized and the endpoint resource constant is appropriately defined.
48-52: Review comment is incorrect—schema validation prevents the concern.The
MilitaryAuditRequestSchemavalidates thatmilitaryStatusis one of['approved', 'declined'](line 30 in api.py), ensuring only valid enum values reach line 52. SinceMilitaryAuditStatusis aStrEnumwith both'approved'and'declined'as valid members, the conversion at line 52 is safe and will not raise aValueError.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
30-30: LGTM!Import addition for
MilitaryAuditStatusis correctly placed with the related enum imports.
698-739: LGTM!The provider update record creation and provider record update with
TENTATIVEstatus are correctly implemented. The previous state capture and transactional update pattern follows the established patterns in the codebase.
837-870: LGTM!The method signature, documentation, and initial validation logic are well-structured. Appropriate use of
CCNotFoundExceptionwhen no military affiliation records exist for the provider.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (2)
43-46: Consider simplifying the UUID conversion.Since
provider_idis already validated as a UUID byMilitaryAuditEventDetailSchema, the conversionUUID(str(provider_id))is redundant. You can passprovider_iddirectly.🔎 Proposed simplification
try: provider_records = config.data_client.get_provider_user_records( compact=compact, - provider_id=UUID(str(provider_id)), + provider_id=provider_id, )
67-69: Consider adding a defensive check for unexpected audit result values.The ternary expression assumes
audit_resultis either 'approved' or 'declined'. While the schema validation should enforce this, adding an explicit validation or assertion would make the code more defensive against future schema changes.🔎 Optional defensive check
# Determine event type and send appropriate notification + if audit_result not in ('approved', 'declined'): + raise CCInternalException(f'Unexpected audit result: {audit_result}') + event_type = ( EventType.MILITARY_AUDIT_APPROVED if audit_result == 'approved' else EventType.MILITARY_AUDIT_DECLINED )backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
799-807: Consider refactoring to eliminate duplication.The method is correct, but the sorting logic duplicates
get_latest_military_affiliation_status(lines 769-782).🔎 Proposed refactor to use get_latest_military_affiliation
Refactor
get_latest_military_affiliation_statusto use this method:def get_latest_military_affiliation_status(self) -> str | None: """ Determine the provider's latest military affiliation status if present. :return: The military affiliation status of the provider if present, else None """ - if not self._military_affiliation_records: - return None - - # we only need to check the most recent military affiliation record - latest_military_affiliation = sorted( - self._military_affiliation_records, key=lambda x: x.dateOfUpload, reverse=True - )[0] - - return latest_military_affiliation.status + latest = self.get_latest_military_affiliation() + return latest.status if latest else None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (2)
MilitaryAffiliationData(10-51)dateOfUpload(46-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (7)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (4)
1-10: LGTM!The imports are well-organized and include all necessary components for event handling, schema validation, and notification tracking.
71-92: LGTM! Well-structured notification logic.The notification logic correctly:
- Differentiates between approved and declined notifications
- Includes
audit_noteonly for declined notifications (intentional and appropriate)- Records success with proper metadata (recipient type, provider ID, event type, and event time)
- Uses structured logging for observability
93-102: LGTM! Robust error handling.The error handling properly:
- Logs the exception with structured context
- Records the failure in the tracker with complete metadata
- Re-raises the exception to trigger SQS retry mechanisms
1-102: LGTM! Well-architected notification listener.The implementation demonstrates solid design:
- Clear separation of concerns (validation → retrieval → idempotency → notification → tracking)
- Proper idempotency handling to prevent duplicate notifications
- Comprehensive error handling with structured logging
- Appropriate use of the tracker pattern for state management
- Good defensive programming (email validation, error recovery)
The minor issues flagged are refinements rather than blockers. The code is production-ready.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
18-18: LGTM!The
MilitaryAuditStatusimport is correctly added and used for defaulting the military status field in the API response.
784-797: LGTM!The method correctly implements the filtering pattern consistent with similar methods in this class. The docstring has been appropriately updated to reflect that it returns a list of records.
958-962: LGTM!The default values ensure backwards compatibility for existing provider records that don't have military audit status fields. Using
NOT_APPLICABLEand empty string as defaults is appropriate.
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
513-519: Consider enhancing email content with actionable information.The current email body is clear but minimal. Consider adding:
- What this approval means for the provider
- Any next steps they should take
- A link to view their account/documentation status
This would improve the user experience by providing more context and guidance.
💡 Example enhancement
const report = this.getNewEmailTemplate(); const subject = 'Military Documentation Approved - Compact Connect'; -const bodyText = 'This message is to notify you that your military documentation has been reviewed and approved by the compact administration.'; +const loginUrl = `${environmentVariableService.getUiBasePathUrl()}/Dashboard`; +const bodyText = `This message is to notify you that your military documentation has been reviewed and approved by the compact administration.\n\nYou can now proceed with your license application. View your account status at:\n\n${loginUrl}`;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.tsbackend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.tsbackend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T16:02:55.832Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:142-161
Timestamp: 2025-08-22T16:02:55.832Z
Learning: In CompactConnect smoke tests, static emails for test staff users are intentionally used because the test framework can detect if a user already exists with that email and will reuse the existing user (who already has the needed permissions configured) rather than failing on username collisions.
Applied to files:
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
backend/compact-connect/lambdas/nodejs/lib/models/email-notification-service-event.ts (1)
EmailNotificationEvent(10-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
370-378: LGTM! Implementation follows established patterns.The new case for military audit approved notifications is well-structured and consistent with other notification handlers in the switch statement. Error handling and parameter passing are appropriate.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
495-524: LGTM! Clean implementation following established patterns.The method is well-structured and consistent with other notification methods in this service. The email content is clear and professional, error handling is appropriate, and the logging provides good visibility.
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
1506-1563: LGTM! Comprehensive test coverage.The test suite effectively covers both the success and failure scenarios for the military audit approved notification. The tests validate all critical aspects:
- Email sending with correct recipients
- Proper subject line
- Expected HTML content
- Error handling for missing recipients
The implementation follows the established testing patterns in this file.
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
1565-1625: LGTM with optional enhancement suggestion.The test suite correctly validates the declined notification flow, including verification that the
auditNoteis present in the email body (line 1612).Optional: Consider adding a test case for when auditNote is empty
While the implementation handles empty
auditNotegracefully (service lines 549-551), adding a test case would document this behavior:it('should send email without auditNote when not provided', async () => { const eventWithoutNote: EmailNotificationEvent = { ...SAMPLE_MILITARY_AUDIT_DECLINED_NOTIFICATION_EVENT, templateVariables: {} }; const response = await lambda.handler(eventWithoutNote, {} as any); expect(response).toEqual({ message: 'Email message sent' }); const emailCall = mockSESClient.commandCalls(SendEmailCommand)[0]; const htmlContent = emailCall.args[0].input.Content?.Simple?.Body?.Html?.Data; expect(htmlContent).toBeDefined(); expect(htmlContent).toContain('This message is to notify you that your military documentation has been reviewed and declined'); expect(htmlContent).not.toContain('Message from the compact administration'); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.tsbackend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.tsbackend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
backend/compact-connect/lambdas/nodejs/lib/models/email-notification-service-event.ts (1)
EmailNotificationEvent(10-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
1506-1563: LGTM!The test suite for military audit approved notifications is well-structured and follows established patterns in the codebase. It correctly validates the email sending success path and the no-recipients error case.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (2)
496-524: LGTM!The implementation of the military audit approved notification follows established patterns in the service and includes appropriate error handling. The notification message is clear and professional.
526-560: LGTM!The implementation properly handles the declined notification with optional
auditNote. The defensive check at line 549 (auditNote.trim().length > 0) ensures empty or whitespace-only notes are not appended to the email body.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
339-353: Good coverage for end‑military‑affiliation behavior; a couple of small test polish opportunitiesThe refactor to
_generate_path_api_eventand the new tests for:
- removing
militaryStatus/militaryStatusNotefrom the provider record, and- asserting a
ProviderUpdateDatawithUpdateCategory.MILITARY_AFFILIATION_ENDED, capturedprevious,removedValues, and emptyupdatedValuesall correctly exercise the new
DataClient.end_military_affiliationbehavior and look solid.Two minor test nits you may want to address:
test_patch_provider_military_affiliation_returs_400_if_invalid_body: spelling (returns) in the test name.test_patch_provider_military_affiliation_creates_provider_update_record: you assertremovedValues == ['militaryStatus', 'militaryStatusNote']. If the implementation ever changes the order in which these are appended, this will become brittle; asserting equality as a set (or sorting before comparison) would make the test more resilient while preserving intent.Also applies to: 410-497
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (3)
674-772:complete_military_affiliation_initialization: logic looks good; consider type consistency and shared batching helperThe refactor to use
ProviderUserRecordsand to couple the “self‑healing” affiliation status update with aMILITARY_FILE_UPLOADprovider update +militaryStatus = TENTATIVEis coherent and matches the new workflow.A couple of small refinements you might consider:
provider_idtype consistency: this method now calls
self.get_provider_user_records(compact=compact, provider_id=UUID(provider_id))and also passesUUID(provider_id)intoProviderUpdateData. Elsewhere in this class,provider_idis usually passed through as the existing value (often astr). Sinceget_provider_user_recordsjust interpolatesprovider_idinto the PK string, casting toUUIDis not strictly necessary and will raise if a non‑UUID provider id ever shows up. Aligning this method with the existing pattern (i.e., using the providedprovider_idas‑is) would avoid that assumption and keep the public surface more uniform.- Batching pattern duplication: the batched
transact_write_itemsloop here is very similar to_execute_batched_transactions. If you find yourself adding more multi‑item flows, it might be worth reusing that helper for consistency and to keep logging/exception behavior centralized.These are non‑blocking but would improve consistency and reduce surprise around
provider_idhandling.
822-933:end_military_affiliation: behavior matches requirements; consider suppressing no‑op update recordsFunctionally this method does what we need:
- Removes
militaryStatus/militaryStatusNotefrom the provider record (withdateOfUpdateset),- Marks all INITIALIZING/ACTIVE military affiliation records as INACTIVE, and
- Persists a
ProviderUpdateDatawithupdateType = MILITARY_AFFILIATION_ENDED,previousstate, andremovedValuesfor the cleared fields.Two minor behavioral nits you may want to tweak:
No‑op updates: When neither
militaryStatusnormilitaryStatusNoteis present,removed_valuesstays empty, but we still create and write a provider update record withupdatedValues = {}and noremovedValues. If the goal is to audit actual state changes, you might want to skip creating the update record entirely whenremoved_valuesis empty:Optional diff sketch to only emit updates when something changed
removed_values = [] if previous_provider_state.get('militaryStatus') is not None: removed_values.append('militaryStatus') if previous_provider_state.get('militaryStatusNote') is not None: removed_values.append('militaryStatusNote')
- update_record_data = {
- update_record_data: dict = {
'type': ProviderRecordType.PROVIDER_UPDATE,
'updateType': UpdateCategory.MILITARY_AFFILIATION_ENDED,
'providerId': provider_id,
'compact': compact,
'previous': previous_provider_state,
'createDate': now,
'updatedValues': {},
}
- if removed_values:
update_record_data['removedValues'] = removed_values- provider_update_record = ProviderUpdateData.create_new(update_record_data)
- if removed_values:
update_record_data['removedValues'] = removed_valuesprovider_update_record = ProviderUpdateData.create_new(update_record_data)- else:
provider_update_record = None
Create provider update record
- transaction_items.append(
{'Put': {'TableName': self.config.provider_table_name,'Item': TypeSerializer().serialize(provider_update_record.serialize_to_database_record())['M'],}}- )
Create provider update record, if any fields actually changed
- if provider_update_record is not None:
transaction_items.append({'Put': {'TableName': self.config.provider_table_name,'Item': TypeSerializer().serialize(provider_update_record.serialize_to_database_record())['M'],}})</details>
- Timestamp consistency: you already compute
now = config.current_standard_datetime, but the providerUpdateExpressionusesself.config.current_standard_datetime.isoformat()again. Reusing the samenowinstance for both the update recordcreateDateand the providerdateOfUpdatewill avoid tiny timing skew between those timestamps.Neither change is required for correctness, but they would make the audit trail a bit cleaner.
950-1040:process_military_audit: core flow is sound; align docstring with behavior and consider stricter preconditionsThe implementation here is straightforward and matches the intended workflow:
- Ensures there is at least one military affiliation via
get_latest_military_affiliation()(failing withCCNotFoundExceptionif not),- Captures the previous provider state,
- Writes a
ProviderUpdateDatawithupdateType = MILITARY_AUDITand updatedmilitaryStatus/militaryStatusNote,- Applies the same values to the provider record, and
- Commits both in a single transaction.
Two small cleanups:
- The docstring says “Update provider and latest military affiliation with audit result in a transaction,” but the code only updates the provider + provider update record; the affiliation record is read for existence but not mutated. Either update the docstring to reflect the actual behavior, or, if you do intend to reflect audit state on the affiliation record itself, wire that in explicitly.
- If you want to be extra defensive, you could assert that the “latest” affiliation is in an expected status (e.g., ACTIVE) before allowing an APPROVED/DECLINED audit, to avoid accidentally auditing an obviously stale/inactive record in edge cases.
Overall the flow looks correct; these are just clarity/guardrail enhancements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
📚 Learning: 2025-08-25T19:04:48.800Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:288-293
Timestamp: 2025-08-25T19:04:48.800Z
Learning: In the CompactConnect project, landonshumway-ia has already provided feedback on patch cleanup patterns in test setup methods for the account recovery tests, so similar suggestions about stopping patches in setUp/tearDown should not be repeated.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
data_client(42-45)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
end_military_affiliation(823-933)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
296-303: Military audit enums and update categories look consistent and fit existing patternsThe new
MilitaryAuditStatusenum and the additionalUpdateCategorymembers align with existing naming/value conventions and with how they’re consumed inDataClient(formilitaryStatusand provider update records). No issues from a schema/API-compatibility perspective.Also applies to: 305-312
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (1)
187-201: PATCH handler correctly wired to newend_military_affiliationflowSwitching from
inactivate_military_affiliation_statustoend_military_affiliationkeeps the public PATCH behavior the same while now also cleaning up provider-levelmilitaryStatusfields and emitting an update record. The validation and response contract are preserved and align with the new tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
674-778: Consider edge case: DuplicatedateOfUploadtimestamps.Lines 694-696 use
max()to find the latest military affiliation record bydateOfUpload. If multiple records have identical timestamps (e.g., from rapid successive uploads), the selection becomes non-deterministic. While unlikely, consider adding a secondary sort key (e.g., record ID) to ensure consistent behavior.Optional: Add secondary sort criterion
# Find the latest military affiliation record by dateOfUpload -latest_military_affiliation_record = max( - initializing_military_affiliation_records, key=lambda record: record.dateOfUpload -) +# Sort by dateOfUpload descending, then by a stable secondary key if needed +latest_military_affiliation_record = max( + initializing_military_affiliation_records, + key=lambda record: (record.dateOfUpload, getattr(record, 'sk', '')) +)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧬 Code graph analysis (2)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (2)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)backend/compact-connect/tests/smoke/smoke_common.py (1)
SmokeTestFailureException(13-17)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
MilitaryAuditStatus(296-302)serialize_to_database_record(205-208)update(182-203)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (3)
compact(29-30)compact(194-195)ProviderUpdateData(170-211)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/transaction/__init__.py (2)
compact(47-48)batch(34-36)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (3)
compact(28-29)compact(112-113)status(88-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (3)
compact(30-31)status(50-51)dateOfUpload(46-47)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
get_military_affiliation_records(784-797)get_latest_military_affiliation(799-807)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(261-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
156-167: LGTM! Validation logic correctly verifies the state transition.The validation correctly checks that when military affiliations are patched to inactive, the militaryStatus transitions to 'notApplicable' and the statusNote remains empty. This aligns with the expected behavior of the military audit workflow.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
30-30: LGTM: Import addition is correct.The
MilitaryAuditStatusenum import is properly placed and consistent with the existing import structure.
830-848: Fix type inconsistency forprovider_idparameter.Line 830 declares
provider_id: str, but line 848 passes it toget_provider_user_recordswhich expectsprovider_id: UUID(see line 182). This type mismatch will cause issues at runtime or with type checkers.🔎 Proposed fix
@logger_inject_kwargs(logger, 'compact', 'provider_id') -def end_military_affiliation(self, compact: str, provider_id: str) -> None: +def end_military_affiliation(self, compact: str, provider_id: UUID) -> None: """ End a provider's military affiliation by removing military status fields and deactivating all active records.Or if callers pass strings, convert explicitly:
# Get provider records - provider_user_records = self.get_provider_user_records(compact=compact, provider_id=provider_id) + provider_user_records = self.get_provider_user_records(compact=compact, provider_id=UUID(provider_id))Note:
process_military_audit(line 962) correctly usesprovider_id: UUIDfor consistency.⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1135 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654 Timestamp: 2025-10-15T19:53:00.422Z Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.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/common/cc_common/data_model/data_client.py (1)
1015-1048: LGTM: Past review concern has been addressed.The implementation correctly uses an
Updateexpression (lines 1021-1038) instead ofPutto modify the provider record. This addresses the race condition concern from the past review comment, ensuring that concurrent modifications to other fields won't be overwritten.The transaction atomically:
- Updates provider's
militaryStatusandmilitaryStatusNotevia Update expression- Creates a provider update audit record
- Validates provider existence via
ConditionExpression(line 1037)
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
765-777: Consider adding rollback logic for failed batches.The batch processing modifies
transaction_items(line 770) before executing the transaction (line 772). If a batch fails after previous batches have succeeded, there's no mechanism to rollback the successful batches, potentially leaving data in an inconsistent state.For comparison,
create_provider_privileges(lines 550-570) tracksprocessed_transactionsand implements rollback via_rollback_privilege_transactionsif any batch fails.While hitting the 100-item batch limit is unlikely for military affiliation records, implementing a similar rollback pattern would improve data consistency guarantees and align with existing patterns in this file.
Note: This concern also applies to
end_military_affiliation(lines 927-938).Potential approach
Track processed batches and implement rollback on failure:
processed_batches = [] try: while transaction_items: batch = transaction_items[:batch_size] transaction_items = transaction_items[batch_size:] self.config.dynamodb_client.transact_write_items(TransactItems=batch) processed_batches.append(batch) logger.info('Successfully processed military affiliation initialization batch', batch_size=len(batch)) except ClientError as e: logger.error('Failed to process military affiliation initialization transaction', error=str(e)) # Implement rollback of processed_batches here raise CCAwsServiceException('Failed to complete military affiliation initialization') from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
30-30: LGTM: MilitaryAuditStatus import added.The import correctly adds the MilitaryAuditStatus enum needed for the military audit functionality.
684-696: LGTM: Correct logic for finding and validating latest military affiliation.The refactor correctly:
- Uses
get_provider_user_recordsfor cleaner data retrieval- Validates that initializing records exist (line 691)
- Selects the latest record by
dateOfUploadusingmax()(lines 694-696)
717-746: LGTM: Correct use of Update expression for atomic provider changes.The transaction correctly uses an
Updateexpression instead ofPutto modify only the specific fields (militaryStatus,militaryStatusNote,dateOfUpdate), preventing race conditions where concurrent modifications could be overwritten. TheConditionExpressionensures the provider exists before updating.
829-925: LGTM: Well-structured implementation of military affiliation termination.The
end_military_affiliationmethod correctly:
- Uses consistent reads for transactional integrity (line 848)
- Captures previous state before modifications (line 852)
- Builds
removedValueslist conditionally (lines 862-866)- Uses
REMOVEin UpdateExpression for military fields (line 895)- Creates audit trail via provider update record
- Ensures atomicity within each batch via DynamoDB transactions
The implementation follows established patterns in this file and maintains data consistency guarantees.
Note: The batch rollback concern noted in the previous comment also applies to this method (lines 927-938).
957-1054: LGTM: Clean implementation of military audit processing.The
process_military_auditmethod correctly:
- Validates the military affiliation exists before proceeding (lines 988-990)
- Captures previous provider state for audit trail (line 996)
- Uses empty string as default for optional note (line 993)
- Executes both updates atomically in a single transaction
- Uses Update expression to prevent race conditions (as addressed in previous comment)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
95-95: Fix typo in comment.Line 95: "tenative" should be "tentative".
🔎 Proposed fix
- # check the militaryStatus of the provider to make sure it is now reset to 'tenative' + # check the militaryStatus of the provider to make sure it is now reset to 'tentative'
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
173-254: Consider test scenario realism: auditing with inactive documents.This test runs after
test_military_affiliation_patch_update(line 259), which inactivates all military affiliation documents. The audit test then attempts to audit military records when there may be no active documents.While this might be valid if the API allows status changes regardless of document state, it may not reflect a realistic audit scenario where admins typically review active documents before approving or declining.
Consider one of the following:
- Re-upload a military document at the beginning of this test to ensure there are active documents to audit
- Add a comment explaining that auditing with inactive documents is a valid test case
- Verify whether the API permits military status audits when no active documents exist
🔎 Example: Re-upload before auditing
Add this after line 190:
# Re-upload a document to ensure there are active documents for audit post_body = { 'fileNames': ['military_affiliation.pdf'], 'affiliationType': 'militaryMember', } post_api_response = requests.post( url=get_api_base_url() + '/v1/provider-users/me/military-affiliation', headers=get_provider_user_auth_headers_cached(), json=post_body, timeout=10, ) if post_api_response.status_code != 200: raise SmokeTestFailureException(f'Failed to POST military affiliation. Response: {post_api_response.json()}') # Upload the file and wait for processing # ... (similar to test_military_affiliation_upload logic)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧬 Code graph analysis (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (8)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)backend/compact-connect/tests/smoke/config.py (1)
load_smoke_test_env(100-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (4)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (4)
10-12: LGTM!The new imports are appropriate and necessary for the military audit test functionality.
Also applies to: 15-15
160-171: LGTM!The validation logic correctly verifies that after inactivating all military affiliation records, the provider's militaryStatus is 'notApplicable' and the note is cleared.
260-260: LGTM!The new audit test is correctly added to the main execution flow.
193-199: The military audit endpoint uses@authorize_compact_level_only_action(action=CCPermissionsAction.ADMIN), which only validates that the staff user has the scope{compact}/admin. It does not perform any jurisdiction-based authorization checks. The hardcoded jurisdiction 'oh' in the staff user permissions is test setup data and does not affect whether the endpoint authorizes the request. This same pattern is consistently used in other smoke tests likequery_provider_smoke_tests.py, confirming it is an established and working approach. No change is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
94-105: Validation logic is correct.The militaryStatus and militaryStatusNote checks appropriately verify the provider state after upload.
Note: The typo on line 95 ("tenative" → "tentative") and error message improvements were already flagged in a previous review.
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
236-239: Consider improving function call formatting.The multiline function calls on lines 236-239 and 260-263 have awkward line breaks that reduce readability.
🔎 Suggested formatting improvements
- updated_provider_data = _call_audit_endpoint_and_get_provider_data(compact, - provider_id, - test_staff_user_email, - patch_body) + updated_provider_data = _call_audit_endpoint_and_get_provider_data( + compact, provider_id, test_staff_user_email, patch_body + )And similarly for lines 260-263:
- provider_data_after_approval = _call_audit_endpoint_and_get_provider_data(compact, - provider_id, - test_staff_user_email, - patch_body) + provider_data_after_approval = _call_audit_endpoint_and_get_provider_data( + compact, provider_id, test_staff_user_email, patch_body + )Also applies to: 260-263
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/compact-connect/lambdas/nodejs/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
backend/compact-connect/lambdas/nodejs/package.jsonbackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
📚 Learning: 2025-07-21T20:40:56.491Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/common/requirements.txt:7-0
Timestamp: 2025-07-21T20:40:56.491Z
Learning: In CompactConnect, there is only one lambda layer in use for Python lambdas, and this single layer manages the versions of aws-lambda-powertools, boto3, and botocore dependencies. This eliminates concerns about version skew across multiple lambda layers since all Python lambdas share the same dependency management through this single layer.
Applied to files:
backend/compact-connect/lambdas/nodejs/package.json
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/lambdas/nodejs/package.json
📚 Learning: 2025-08-12T19:49:48.235Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.txt:1-6
Timestamp: 2025-08-12T19:49:48.235Z
Learning: The disaster-recovery Lambda functions in CompactConnect get their aws-lambda-powertools dependency from the shared lambda layer rather than individual requirements.txt files, which is why their requirements.txt files can be empty or header-only.
Applied to files:
backend/compact-connect/lambdas/nodejs/package.json
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/nodejs/package.json
📚 Learning: 2025-11-21T01:00:00.678Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1218
File: backend/compact-connect-ui-app/stacks/frontend_deployment_stack/distribution.py:127-127
Timestamp: 2025-11-21T01:00:00.678Z
Learning: As of November 2025, AWS Lambda and LambdaEdge support Node.js 24.x runtime (Runtime.NODEJS_24_X in AWS CDK), even though official documentation may not be fully updated yet.
Applied to files:
backend/compact-connect/lambdas/nodejs/package.json
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧬 Code graph analysis (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
backend/compact-connect/tests/smoke/smoke_common.py (8)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (5)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (4)
10-15: LGTM!The new imports are necessary for the audit test functionality and correctly reference helpers from
smoke_common.py.
160-171: LGTM!The validation correctly verifies that militaryStatus transitions to 'notApplicable' and militaryStatusNote is cleared after deactivating affiliations.
173-199: LGTM!The helper function appropriately encapsulates the audit endpoint call and provider data retrieval with proper error handling.
288-288: LGTM!The new test is correctly added to the main execution block.
backend/compact-connect/lambdas/nodejs/package.json (1)
43-43: Version 2.30.0 is valid and supports Node.js 24.The dependency update is safe. Version 2.30.0 exists in npm, supports Node.js 24 (as evidenced by the @types/node@24.10.2 dependency bump in the release), has no known security vulnerabilities, and contains no breaking changes between 2.10.0 and 2.30.0. The Logger is extensively integrated across the codebase (17+ files), confirming compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
264-268: Fix copy-paste error in comment (still present from previous review).The comment on line 264 still says "Verify militaryStatus is set to declined" but the code checks for 'approved'. This was flagged in a previous review but hasn't been corrected yet.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved':
🧹 Nitpick comments (4)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (3)
173-199: Helper function trades linear narrative for reduced duplication.This helper extracts common logic for calling the audit endpoint and retrieving provider data. While it reduces duplication between the declined and approved test flows, it slightly compromises the linear narrative readability that smoke tests typically favor.
Based on learnings, smoke tests in this project generally prefer keeping logic inline for top-to-bottom readability. However, given the minimal scope and the clear naming, this trade-off seems reasonable.
Based on learnings, smoke tests should prioritize linear narrative readability over complexity reduction.
236-239: Fix inconsistent line continuation formatting.The function call arguments have inconsistent indentation. The opening argument should align with the opening parenthesis or be indented consistently.
🔎 Proposed formatting fix
- updated_provider_data = _call_audit_endpoint_and_get_provider_data(compact, - provider_id, - test_staff_user_email, - patch_body) + updated_provider_data = _call_audit_endpoint_and_get_provider_data( + compact, + provider_id, + test_staff_user_email, + patch_body + )Apply similar formatting to lines 260-263:
- provider_data_after_approval = _call_audit_endpoint_and_get_provider_data(compact, - provider_id, - test_staff_user_email, - patch_body) + provider_data_after_approval = _call_audit_endpoint_and_get_provider_data( + compact, + provider_id, + test_staff_user_email, + patch_body + )Also applies to: 260-263
270-275: Clarify misleading comment about empty militaryStatusNote.The comment states "Verify militaryStatusNote equals what was passed into the request body", but no
militaryStatusNotewas included in the approval request body (lines 257-259). The test is actually verifying that the note is cleared/empty after approval.🔎 Proposed clarification
- # Verify militaryStatusNote equals what was passed into the request body + # Verify militaryStatusNote is empty after approval if provider_data_after_approval.get('militaryStatusNote') != '':backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (1)
42-50: Consider more specific exception handling.The broad
except Exceptionhandler is acceptable since it logs and re-raises, but consider catching more specific exceptions (e.g.,ClientError,ValidationError) if there are known failure modes that warrant different handling or logging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
🧬 Code graph analysis (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
backend/compact-connect/tests/smoke/smoke_common.py (8)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (12)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (5)
94-106: LGTM! Military status validation added.The assertions correctly verify that
militaryStatusis set to 'tentative' andmilitaryStatusNoteis empty after the upload flow completes.
160-171: LGTM! Military status validation after deactivation.The assertions correctly verify that
militaryStatustransitions to 'notApplicable' andmilitaryStatusNoteis empty after deactivating military affiliations.
241-254: LGTM! Declined status audit flow is correctly validated.The test properly verifies that both
militaryStatusandmilitaryStatusNoteare updated to the declined values passed in the PATCH request.
279-281: LGTM! Proper cleanup in finally block.The test correctly ensures the staff user is deleted regardless of whether the test passes or fails, preventing test data accumulation.
288-288: LGTM! New audit test added to execution flow.The new test function is appropriately invoked in the main block.
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (7)
1-7: LGTM!The imports are well-organized and all necessary for the handler's functionality.
10-20: LGTM!The function signature and documentation are clear. The previously flagged
# noqa: ARG001comment has been correctly removed.
21-32: LGTM!Event detail extraction and validation follow the expected pattern using the schema.
52-59: LGTM!Good defensive programming with the email validation check and appropriate exception handling.
61-64: LGTM!The idempotency check correctly uses the tracker to prevent duplicate notifications.
71-102: LGTM with one note!The notification sending logic and error handling pattern are well-structured:
- Clear branching for approved vs. declined notifications
- Proper success/failure tracking with the tracker
- Exception re-raising allows SQS retry/DLQ mechanisms to function correctly
Note: The string literal
'approved'at line 72 should also be replaced with a constant/enum (already flagged at lines 66-69).
34-50: Simplify the redundant UUID conversion.Line 45 performs a double conversion:
UUID(str(provider_id)). TheMilitaryAuditEventDetailSchemadefinesproviderIdas aUUIDfield, which deserializes to a UUID object. Sinceget_provider_user_records()expects aprovider_id: UUIDparameter, the conversion is redundant. Simplify toprovider_id=provider_id.⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1135 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654 Timestamp: 2025-10-15T19:53:00.422Z Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1014 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:0-0 Timestamp: 2025-08-21T20:24:39.042Z Learning: When validating identifier fields like providerId in Marshmallow schemas, prefer using the UUID field type over String with Length validation, as UUID provides automatic format validation and is more semantically appropriate.Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 796 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64 Timestamp: 2025-06-03T16:41:07.757Z Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.pybackend/compact-connect/lambdas/python/common/tests/resources/dynamo/military-affiliation.jsonbackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/resources/dynamo/military-affiliation.json
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_top_level_record(1137-1160)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (11)
compact(29-30)compact(194-195)providerId(25-26)providerId(190-191)militaryStatus(151-158)militaryStatusNote(161-167)ProviderUpdateData(170-211)updateType(186-187)previous(202-203)removedValues(210-211)updatedValues(206-207)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
UpdateCategory(305-324)from_database_record(124-139)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
current_standard_datetime(261-265)data_client(42-45)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
end_military_affiliation(830-940)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (7)
backend/compact-connect/lambdas/python/common/tests/resources/dynamo/military-affiliation.json (1)
3-3: Test data correctly updated to match schema change.The sort key format update from date-only to full ISO timestamp aligns with the schema changes in
record.py. This ensures test data accurately reflects the new format.backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (1)
199-199: Method rename properly implemented.The rename from
inactivate_military_affiliation_statustoend_military_affiliationimproves clarity and aligns with the expanded functionality that removes military status fields and creates provider update records.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (4)
339-348: Good refactoring to eliminate test event duplication.The new
_generate_path_api_eventhelper centralizes PATCH event creation, improving test maintainability and reducing duplication across multiple test cases.
239-239: Test expectation correctly updated for new S3 path format.The expected document key now includes the full ISO timestamp, matching the implementation change in
provider_users.pyline 136.
433-456: Comprehensive test coverage for military status field removal.This test properly verifies that ending a military affiliation removes
militaryStatusandmilitaryStatusNotefrom the provider record. The test setup withvalue_overridesand assertions using the data client accessor methods follow established patterns.
457-497: Thorough validation of provider update record creation.This test comprehensively verifies the
MILITARY_AFFILIATION_ENDEDupdate record:
- Confirms exactly one update record is created
- Validates the update type
- Verifies previous state capture
- Checks that removed values are properly tracked
- Confirms updated values remain empty (since fields were removed, not updated)
The test provides strong coverage of the transactional update logic.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py (1)
39-40: The codebase already handles both sort key formats gracefully—no backward compatibility issues exist.The
begins_with()query on the sort key is format-agnostic and works with both2024-11-08and2024-11-08T23:59:59+00:00formats. Additionally, the code that determines which military affiliation record is newest uses thedateOfUploadfield value for comparison (not sort key parsing), making it immune to format changes. The existing testtest_complete_military_affiliation_initialization_sets_expected_statusexplicitly validates this by creating old-format records (2024-07-08,2024-07-09), querying them, and verifying that the code correctly handles mixed old and new format records in the same result set.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
258-262: Fix copy-paste error in comment.Line 258: The comment says "Verify militaryStatus is set to declined" but the code below checks for 'approved'. This is a copy-paste error from the earlier declined verification.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved': raise SmokeTestFailureException(
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
106-119: Consider explicitly passing provider_id to military affiliation creation.The test creates military affiliation records without explicitly linking them to the provider created on line 99. While the test_data_generator likely handles this linkage implicitly (since line 122 successfully queries by provider_id), making this relationship explicit would improve test clarity and reduce coupling to test_data_generator internals.
Suggested refactor to make provider linkage explicit
- self.test_data_generator.put_default_military_affiliation_in_provider_table( - { - 'status': 'initializing', - 'dateOfUpload': datetime.fromisoformat('2024-07-08T13:34:59+00:00'), - } - ) + self.test_data_generator.put_default_military_affiliation_in_provider_table( + { + 'providerId': provider_id, + 'status': 'initializing', + 'dateOfUpload': datetime.fromisoformat('2024-07-08T13:34:59+00:00'), + } + ) # now add record on following day - self.test_data_generator.put_default_military_affiliation_in_provider_table( - { - 'status': 'initializing', - 'dateOfUpload': datetime.fromisoformat('2024-07-09T10:34:59+00:00'), - } - ) + self.test_data_generator.put_default_military_affiliation_in_provider_table( + { + 'providerId': provider_id, + 'status': 'initializing', + 'dateOfUpload': datetime.fromisoformat('2024-07-09T10:34:59+00:00'), + } + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.pybackend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In the CompactConnect backend, provider data sanitization follows a centralized architecture: the ProviderRecordUtility.generate_api_response_object() method serves as the single source for constructing provider response objects and includes sanitization of sensitive profile fields. Downstream functions like sanitize_provider_data_based_on_caller_scopes() in utils.py receive already-sanitized data, making additional sanitization calls redundant. This ensures sensitive fields are removed at the source rather than requiring multiple sanitization points throughout the codebase.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: The CompactConnect provider data sanitization architecture follows this flow: endpoints call get_provider_information() → which calls provider_user_records.generate_api_response_object() → which includes sanitization of sensitive profile fields (emailVerificationExpiry, emailVerificationCode, pendingEmailAddress, recoveryToken, recoveryExpiry) → sanitized data then flows to sanitize_provider_data_based_on_caller_scopes() for additional scope-based filtering. This centralized approach ensures sensitive fields are removed at the source rather than requiring sanitization at multiple return points.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In CompactConnect's provider data architecture, sanitization of sensitive profile fields (emailVerificationExpiry, emailVerificationCode, pendingEmailAddress, recoveryToken, recoveryExpiry) occurs centrally in ProviderRecordUtility.generate_api_response_object(). All endpoints use this method either directly or through get_provider_information(), ensuring that downstream functions like sanitize_provider_data_based_on_caller_scopes() receive already-sanitized data. This eliminates the need for additional sanitization calls at individual return points throughout the codebase.
Applied to files:
backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (13)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
99-100: Good refactoring to use test data generator.The shift from loading static JSON to using the test data generator improves test maintainability and makes the test setup more explicit.
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (7)
10-15: LGTM! New imports are appropriate.The imported functions are all used in the new audit test workflow.
94-106: LGTM! Status validation is correct.The checks appropriately verify that after file upload,
militaryStatustransitions to 'tentative' andmilitaryStatusNoteis empty.
160-171: LGTM! Status validation after PATCH is correct.The checks appropriately verify that after marking affiliations inactive,
militaryStatustransitions to 'notApplicable' andmilitaryStatusNoteis empty.
173-197: LGTM! Helper function appropriately reduces duplication.The function correctly uses staff authentication for the PATCH audit endpoint and provider authentication for retrieving updated data, with appropriate error handling.
199-252: LGTM! Declined status audit flow is well-structured.The test correctly creates a staff user with appropriate permissions, calls the audit endpoint with declined status, and verifies both the status and note are updated as expected.
264-276: LGTM! Approved status verification and cleanup are correct.The verification correctly checks that
militaryStatusis 'approved' andmilitaryStatusNoteis empty. The finally block ensures proper cleanup of the test staff user.
282-282: LGTM! New test correctly added to execution flow.The new audit test is appropriately invoked alongside the existing smoke tests.
backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json (2)
6-7: LGTM: New military audit fields added.The addition of
militaryStatusandmilitaryStatusNotealigns with the PR objectives to support military document auditing by compact directors.
90-90: The document key format in this test resource file is correct and consistent with production code.The timestamp format
2024-07-08T23:59:59+00:00in the documentKeys path matches the production code inprovider_users.pywhich usesconfig.current_standard_datetime.isoformat(). This returns a full ISO 8601 datetime with UTC timezone, not just a date. The:characters in the timestamp are automatically URL-encoded by boto3'sgenerate_presigned_url()method when generating download links, so no manual encoding is required. While some test code files (e.g.,test_provider_s3_events.py) use date-only format for test data, the actual implementation and the file being reviewed use the correct full ISO 8601 format consistently.backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py (3)
48-48: LGTM: Properly captures documentKeys for test setup.The extraction of
documentKeysfrom the test fixture correctly supports the subsequent test logic that verifies these keys are sanitized for non-private scopes.
53-53: LGTM: Test correctly simulates DB data structure.Adding
documentKeysback to the loaded provider after schema loading properly simulates the complete data structure that would come from the database, enabling accurate sanitization testing.
63-66: LGTM: Properly tests sanitization of military audit fields.The test correctly verifies that both
militaryStatusNoteanddocumentKeysfrom military affiliations are removed when the caller lacks read private scope, which aligns with the security requirements for the military audit feature.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
258-262: Fix copy-paste error in comment.The comment on line 258 says "Verify militaryStatus is set to declined" but the code checks for 'approved'. This should be corrected to match the actual verification being performed.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved': raise SmokeTestFailureException( f'Military status is not approved. Status: {provider_data_after_approval.get("militaryStatus")}' )
🧹 Nitpick comments (2)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
264-269: Optional: Clarify comment about expected empty note.The comment states "Verify militaryStatusNote equals what was passed into the request body", but the request body on line 254 doesn't include
militaryStatusNote. The actual expectation is that approving without a note should clear any existing note to an empty string.🔎 Suggested clarification
- # Verify militaryStatusNote equals what was passed into the request body + # Verify militaryStatusNote is cleared to empty string when no note is provided if provider_data_after_approval.get('militaryStatusNote') != '':backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
957-1054: LGTM! Atomic updates ensure transaction safety.The implementation correctly uses
Updateexpressions (lines 1021-1038) to atomically modify only the required fields, preventing the race condition flagged in the previous review. The transaction properly combines the provider update with the provider_update record creation.Optional consideration: You might want to validate that
latest_military_affiliationis in an expected status (e.g.,ACTIVEorTENTATIVE) before processing the audit, to prevent auditing already-declined or inactive records. However, this depends on your business requirements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧬 Code graph analysis (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (8)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (9)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (7)
10-15: LGTM!The new imports are necessary for the audit test workflow and properly sourced from smoke_common.
94-114: LGTM!The added assertions verify the expected military status state after upload, and the refactored date selection using
max()with akeyfunction is cleaner and more robust than string-based date filtering.
160-171: LGTM!The assertions correctly verify that deactivating military affiliations resets the military status to 'notApplicable' with an empty note, maintaining consistency with the expected state transitions.
173-197: LGTM!The helper function is well-structured, properly switches between staff and provider authentication contexts, and effectively reduces code duplication in the audit test scenarios.
199-257: LGTM!The audit test properly validates the decline workflow, correctly verifies both the status and the note fields, and follows the established pattern for smoke tests as narrative functions.
273-276: LGTM!The finally block ensures proper cleanup of test resources, which is essential for smoke test reliability.
282-282: LGTM!The new audit test is properly integrated into the smoke test suite execution flow.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
674-779: LGTM! Transaction safety and batch processing implemented correctly.The refactored implementation properly addresses the race condition from the previous review by using
Updateexpressions instead ofPutoperations (lines 721-739). The batch processing logic (lines 765-777) correctly handles DynamoDB's 100-item transaction limit with appropriate error handling.
829-941: Critical: Type conversion missing forprovider_id.Line 848 passes
provider_id(typed asstr) directly toget_provider_user_records, which expectsUUID(line 182). This will cause a runtime type error.Compare with
complete_military_affiliation_initialization(line 684) andprocess_military_audit(line 984), which handle this correctly.🔎 Proposed fix
logger.info('Ending military affiliation for provider') # Get provider records - provider_user_records = self.get_provider_user_records(compact=compact, provider_id=provider_id) + provider_user_records = self.get_provider_user_records(compact=compact, provider_id=UUID(provider_id)) provider_record = provider_user_records.get_provider_record()⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1135 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654 Timestamp: 2025-10-15T19:53:00.422Z Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
landonshumway-ia
force-pushed
the
feat/military-audit-status
branch
from
4fe4976 to
1234aae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
258-258: Fix copy-paste error in comment (duplicate issue).The comment still says "Verify militaryStatus is set to declined" but the code checks for 'approved'. This was previously flagged and marked as addressed in commit 72f926d, but appears to have been reintroduced.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved':
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
526-560: LGTM! Declined notification method correctly implemented with proper auditNote handling.The method properly validates recipients, conditionally includes the admin's audit note (lines 549-551), and follows the established email template pattern. The defensive check for
auditNote.trim().length > 0ensures empty or whitespace-only notes aren't appended to the email body.One optional enhancement to consider: the email body could provide guidance on next steps for the provider (e.g., "Please review the feedback and resubmit your documentation" or contact information for questions). However, the current implementation meets the PR requirements.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
14-78: Handler correctly wires validation, data update, and event publishingAuthorization, request validation, audit status mapping, data_client call, and event_bus publishing are all consistent with existing patterns and the new schema types. Only optional hardening would be to treat a missing
event['body']explicitly as a 400 instead of relying on a TypeError fromjson.loads, but this matches patterns elsewhere and isn’t strictly required.backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (1)
11-103: Listener correctly wires event parsing, idempotency, and email dispatchThe handler cleanly validates the event detail, looks up the registered provider email, enforces idempotency via
tracker.should_send_provider_notification, and dispatches to the appropriate email client method withEventTyperecorded on success/failure. Enum-based checks againstMilitaryAuditStatus.APPROVEDare acceptable given the StrEnum-style usage, but if the schema ever starts returning raw strings, comparing to.APPROVED.valuewould be a low-risk future-hardening tweak.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
829-941:end_military_affiliationtransactional behavior matches tests; minor doc nuanceThe method correctly:
- Captures the previous provider state,
- Creates a
MILITARY_AFFILIATION_ENDEDProviderUpdateDatawithremovedValueswhen prior fields existed,- Removes
militaryStatus/militaryStatusNotefrom the provider record, and- Marks all
INITIALIZING/ACTIVEmilitary affiliation recordsINACTIVEin batchedtransact_write_itemscalls.This aligns with the new tests and overall design. Note that the docstring says “All operations are performed in a DynamoDB transaction”, but batching can result in multiple transactions for large providers; consider softening that wording to “one or more DynamoDB transactions” to avoid overpromising atomicity at very high record counts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.tsbackend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.tsbackend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.tsbackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/common/cc_common/event_bus_client.pybackend/compact-connect/lambdas/python/common/cc_common/event_state_client.pybackend/compact-connect/lambdas/python/common/common_test/test_data_generator.pybackend/compact-connect/lambdas/python/common/tests/function/test_data_client.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.jsonbackend/compact-connect/lambdas/python/common/tests/resources/dynamo/military-affiliation.jsonbackend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.pybackend/compact-connect/stacks/api_lambda_stack/provider_management.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/stacks/api_stack/v1_api/provider_management.pybackend/compact-connect/stacks/notification_stack.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🚧 Files skipped from review as they are similar to previous changes (18)
- backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py
- backend/compact-connect/lambdas/python/common/tests/resources/dynamo/military-affiliation.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py
- backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
- backend/compact-connect/stacks/notification_stack.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/common/tests/resources/api/provider-detail-response.json
- backend/compact-connect/lambdas/python/common/tests/unit/test_sanitize_provider_data.py
- backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_data_model/test_provider_transformations.py
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
- backend/compact-connect/stacks/api_stack/v1_api/provider_management.py
- backend/compact-connect/lambdas/python/common/cc_common/event_state_client.py
- backend/compact-connect/stacks/api_lambda_stack/provider_management.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.py
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.pybackend/compact-connect/lambdas/python/common/cc_common/email_service_client.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-08-14T20:56:56.938Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:5698-5700
Timestamp: 2025-08-14T20:56:56.938Z
Learning: In the CompactConnect project, there is a deliberate design pattern for API schemas: request schemas should include enum validation constraints (like encumbranceType with its allowed values), while response schemas should remain unconstrained (just "type": "string") to allow flexibility in API responses.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-08-21T20:24:39.042Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:0-0
Timestamp: 2025-08-21T20:24:39.042Z
Learning: When validating identifier fields like providerId in Marshmallow schemas, prefer using the UUID field type over String with Length validation, as UUID provides automatic format validation and is more semantically appropriate.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: The datetime.fromisoformat() method in the CompactConnect DSA authentication module already supports both 'Z' and '+00:00' ISO 8601 timestamp formats, as verified by existing tests. The timezone-naive timestamp handling should be added as an additional safety measure.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-12-01T17:57:35.454Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/tests/app/test_search_persistent_stack.py:104-130
Timestamp: 2025-12-01T17:57:35.454Z
Learning: In backend/compact-connect/tests/app/test_search_persistent_stack.py, the OpenSearch Domain's EncryptionAtRestOptions.KmsKeyId assertion correctly uses {"Ref": encryption_key_logical_id} format. This pattern has been validated to work with CDK's opensearchservice.Domain construct.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-08-25T22:31:44.837Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1005
File: backend/compact-connect/lambdas/python/common/cc_common/dsa_auth.py:212-228
Timestamp: 2025-08-25T22:31:44.837Z
Learning: In cc_common.dsa_auth._validate_dsa_signature, ensure naive datetimes are treated as UTC (set tzinfo=UTC) before comparing to now(UTC) to avoid aware-vs-naive TypeError.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
📚 Learning: 2025-04-29T15:30:08.849Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py:17-23
Timestamp: 2025-04-29T15:30:08.849Z
Learning: The MilitaryAffiliationData class in CompactConnect is explicitly designed with read-only properties as stated in its docstring, even though it inherits from CCDataClass which supports initialization without data followed by using setters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py
🧬 Code graph analysis (11)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
MilitaryAuditStatusField(136-138)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (10)
webroot/src/plugins/EnvConfig/envConfig.plugin.ts (1)
config(60-88)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
CCPermissionsAction(224-234)MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py (1)
MilitaryAuditRequestSchema(27-31)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (3)
api_handler(90-212)to_uuid(1026-1042)get(86-87)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
compact(29-30)compact(194-195)webroot/src/components/App/App.ts (1)
messages(77-79)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
data_client(56-59)event_bus_client(346-349)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
process_military_audit(958-1053)backend/compact-connect/lambdas/python/common/cc_common/event_bus_client.py (1)
publish_military_audit_event(550-587)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (8)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
pop(77-78)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py (7)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (3)
put_default_provider_record_in_provider_table(477-497)put_default_military_affiliation_in_provider_table(239-253)generate_test_api_event(35-63)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (10)
compact(29-30)compact(194-195)providerId(25-26)providerId(190-191)militaryStatus(151-158)militaryStatusNote(161-167)ProviderUpdateData(170-211)updateType(186-187)previous(202-203)updatedValues(206-207)backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
military_audit_handler(16-78)webroot/src/plugins/EnvConfig/envConfig.plugin.ts (1)
config(60-88)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_top_level_record(1137-1160)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
UpdateCategory(305-324)from_database_record(124-139)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
ForgivingSchema(21-25)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (2)
compact(58-59)compact(108-109)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
Compact(51-53)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (2)
MilitaryAffiliationData(10-51)dateOfUpload(46-47)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (4)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (1)
provider_users_api_handler(30-59)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
get_provider_top_level_record(1137-1160)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (6)
compact(29-30)compact(194-195)providerId(25-26)providerId(190-191)militaryStatus(151-158)militaryStatusNote(161-167)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
UpdateCategory(305-324)from_database_record(124-139)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
MilitaryAuditStatus(296-302)UpdateCategory(305-324)update(182-203)backend/compact-connect/tests/smoke/smoke_common.py (1)
get_provider_user_records(254-281)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (3)
compact(29-30)compact(194-195)ProviderUpdateData(170-211)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (3)
compact(28-29)compact(112-113)status(88-92)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (3)
compact(30-31)status(50-51)dateOfUpload(46-47)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (4)
get_provider_record(83-88)get_provider_record(696-705)ProviderRecordType(31-44)get_latest_military_affiliation(799-807)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(275-279)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)
backend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py (2)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (1)
military_audit_notification_listener(12-103)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
put_default_provider_record_in_provider_table(477-497)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (41)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
370-388: LGTM! Both notification cases correctly implemented.The military audit approved and declined notification cases properly validate recipients, call the appropriate email service methods, and follow the established patterns in this handler. The declined notification correctly passes the optional
auditNotefromtemplateVariableswith a safe fallback to an empty string.This implementation addresses the PR objective to "send automatic notification to the licensee when a document is approved or declined" and resolves the past review comment about the missing declined notification case.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
496-524: LGTM! Approved notification method correctly implemented.The method follows established patterns in this service, properly validates recipients, and uses clear, concise email content. The implementation aligns with other notification methods in the file and matches the signature expected by the Lambda handler.
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
12-12: LGTM: Import addition is clean.The MilitaryAuditStatus import follows the established pattern and integrates well with the existing common schema imports.
342-357: Excellent separation of concerns between upload and audit status.The refactored logic properly distinguishes between:
- Upload status (line 343): Verifies document upload succeeded using
get_latest_military_affiliation_status()and blocks on INITIALIZING- Audit status (lines 354-357): Derives
provider_military_statusfrom the provider record with defensive NOT_APPLICABLE default, then determines active military eligibility based on TENTATIVE or APPROVED statesThis dual-check approach ensures both document processing completion and audit approval are validated before granting military benefits. The default handling at line 354 is appropriately defensive.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (4)
303-328: Well-refactored test helper aligned with new architecture.The renamed
_when_testing_military_statushelper now correctly setsmilitaryStatuson the provider record (lines 315-318) rather than loading a separate military affiliation record. This aligns with the new approach where audit status is stored directly on the provider record. The helper properly derivesactive_militaryattestations from the expected parameter.
330-355: Comprehensive test coverage for all military audit states.The test suite now covers all four MilitaryAuditStatus values:
tentative→ active military (line 334)approved→ active military (line 341)declined→ not active military (line 348)notApplicable→ not active military (line 355)This thorough coverage validates the status interpretation logic and ensures the correct attestation requirements are enforced for each state.
358-379: INITIALIZING test correctly uses affiliation record.This test appropriately continues to use
_load_military_affiliation_record_data(status='initializing')(line 365) because it's validating the document upload/processing status (MilitaryAffiliationStatus), not the audit status (MilitaryAuditStatus). The two status types serve distinct purposes, and this test correctly targets the upload status validation.
969-1004: Test updated consistently with new military status pattern.The test now sets
militaryStatus: 'approved'on the provider record (lines 978-980) rather than loading a separate military affiliation record. This validates that the handler correctly requires military attestations when the audit status indicates active military affiliation. The test flow properly verifies both the rejection without attestations and success with attestations.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/record.py (1)
39-40: No action needed. The sort key format change from date to datetime is fully backwards compatible with existing query patterns. All military affiliation record queries useKey('sk').begins_with(f'{compact}#PROVIDER#military-affiliation#'), which matches both date-only and datetime formats since they share the same prefix. No records will be unretrievable, and no migration is required.Likely an incorrect or invalid review comment.
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (7)
10-15: LGTM!The new imports are appropriate and necessary for the military affiliation audit test functionality.
95-105: LGTM!The validation correctly ensures that after uploading military documentation, the provider's status is set to 'tentative' with an empty note. The success message improves test feedback.
110-114: LGTM!Using
max()withdateOfUploadis more robust than date-string matching, as it reliably selects the most recent upload regardless of timing issues.
160-170: LGTM!The validation correctly ensures that after marking all military affiliations as inactive, the provider's status transitions to 'notApplicable' with an empty note.
173-196: LGTM!The helper function appropriately reduces code duplication while maintaining readability. It correctly handles authentication context switching between staff user (for audit) and provider user (for verification), which is consistent with the smoke test patterns in this codebase.
199-257: LGTM!The audit test function is well-structured and comprehensive:
- Properly creates and cleans up test resources using a finally block
- Tests both decline (with note) and approve (without note) workflows
- Validates that approval clears any existing status notes
- Follows the established smoke test patterns for this codebase
Based on learnings, the single-function approach with the helper for repeated operations maintains narrative readability while avoiding duplication.
Also applies to: 259-276
282-282: LGTM!The new audit test is correctly integrated into the main execution flow.
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (3)
18-18: LGTM!Import of
MilitaryAuditStatusis correctly added to support the default value assignment ingenerate_api_response_object.
784-807: LGTM!Both new methods follow the established patterns in this class:
get_military_affiliation_recordsmirrors other getter methods with optional filter conditionsget_latest_military_affiliationcorrectly sorts bydateOfUploadin descending order and returns the first element
958-962: LGTM!The default value logic ensures backwards compatibility by providing sensible defaults when these fields are missing from existing provider records. Using
MilitaryAuditStatus.NOT_APPLICABLE.valuealigns with the enum definition incommon.py.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (1)
457-458: LGTM!The test correctly removes
militaryStatusandmilitaryStatusNotefrom the expected public response. This aligns withProviderPublicResponseSchemainprovider/api.py, which intentionally excludes military status fields from public endpoints to protect sensitive audit information.backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
134-137: LGTM!Using the full ISO 8601 timestamp (
config.current_standard_datetime.isoformat()) instead of date-only provides better uniqueness for S3 document keys and improved traceability for audit purposes.
199-199: LGTM!The method rename from
inactivate_military_affiliation_statustoend_military_affiliationprovides clearer semantic meaning for the operation being performed.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
87-96: LGTM!The
MilitaryAuditEventDetailSchemacorrectly:
- Inherits from
ForgivingSchemainstead ofDataEventDetailBaseSchemasince military audits operate at the provider level without jurisdiction context- Restricts
auditResultto['approved', 'declined']as these are the only valid audit actions (versustentativeandnotApplicablewhich are system-assigned states)- Makes
auditNoteoptional since notes are typically only provided for declined auditsbackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (3)
16-16: LGTM!Import of
MilitaryAuditStatusFieldis correctly added to support the new military status fields in the response schemas.
131-133: LGTM!Adding both
militaryStatusandmilitaryStatusNotetoProviderReadPrivateResponseSchemais appropriate since users with read-private permissions should see the full audit details including any notes.
187-188: LGTM!Good design decision to include only
militaryStatusinProviderGeneralResponseSchemawhile excludingmilitaryStatusNote. The note may contain sensitive details about why documentation was declined, so restricting it to read-private access is appropriate.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py (4)
1-46: LGTM!Well-structured test setup with a reusable helper method
_when_testing_military_auditthat:
- Creates provider and military affiliation records as needed
- Properly constructs test events with appropriate scopes and parameters
- Supports both default and custom provider scenarios
48-104: LGTM!Comprehensive happy-path test coverage:
- Validates approved/declined status returns OK
- Verifies provider record is updated with correct status and note
- Tests both with and without audit notes
106-183: LGTM!Good error case coverage:
- 400 for invalid/missing status values
- 404 when provider has no military affiliation
- 403 for non-admin users
185-223: LGTM!Excellent test for verifying the provider update record creation:
- Sets up initial state with
tentativestatus- Verifies exactly one update record is created
- Validates update type is
MILITARY_AUDIT- Confirms previous state and updated values are correctly captured
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
150-167: LGTM!The new
militaryStatusandmilitaryStatusNoteproperties:
- Follow the existing property patterns in
ProviderData- Use
.get()appropriately since these fields may not exist in older records- Include clear docstrings documenting possible values and defaults
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (4)
2-2: LGTM!Import of
timedeltais needed for the new test that creates military affiliation records with different upload dates.
35-59: LGTM!Good updates to the test setup:
- Pre-populates provider with
DECLINEDstatus and a note to test the transition toTENTATIVE- Uses full ISO 8601 timestamp for S3 keys to match the implementation change in
provider_users.py
87-168: LGTM!Excellent test case for the multiple affiliation records scenario:
- Creates two records with different upload dates (older and newer)
- Processes S3 event for the newer file
- Verifies older record becomes
inactivewhile newer becomesactive- Properly sorts records by
dateOfUploadbefore assertions
170-232: LGTM!Comprehensive tests for provider state updates:
test_provider_user_bucket_event_handler_updates_provider_record_with_tentative_statuscorrectly verifies the transition fromDECLINEDtotentativetest_provider_user_bucket_event_handler_creates_provider_update_recordvalidates the update record captures previous state (DECLINED with note) and updated state (tentative with empty note)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (1)
764-810: Military audit email helpers align with existing payload patternsTemplates, recipient type, and variable shapes are consistent with the other SPECIFIC-recipient methods in this client. No issues from a correctness or safety standpoint.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (3)
231-249: Updated S3 key expectation matches timestamped document keysThe assertion now expects the ISO timestamp (
2024-11-08T23:59:59+00:00) segment in the S3 key, which aligns with the patched behavior in the upload flow and keeps the test in sync with how documentKeys are generated.
339-417: Refactored PATCH event helper and initializing-status test improve clarityUsing
_generate_path_api_eventto build the PATCH event from a real provider record reduces duplicated setup and ensures claims stay in sync with the stored provider. Thetest_patch_provider_military_affiliation_updates_status_when_initializingpath correctly covers the transition frominitializingtoinactive, matching the handler behavior.
433-497: New tests accurately lock in end-of-military-affiliation semanticsThe two tests:
- Confirm top-level
militaryStatusandmilitaryStatusNoteare cleared on the provider record, and- Verify a single
ProviderUpdateDatarecord withUpdateCategory.MILITARY_AFFILIATION_ENDED, correctprevious,removedValues, and emptyupdatedValues,are consistent with the new
DataClient.end_military_affiliationimplementation and give good regression coverage for future changes.backend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py (1)
16-111: Tests give good coverage for success and failure notification pathsThe three tests exercise approved vs declined flows (including passing
audit_note) and the error path where the email client raises, correctly assertingbatchItemFailuresbehavior from the SQS wrapper and verifying the exact call signatures into the email client. This is solid functional coverage for the new listener.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
30-30: Military audit status integration into initialization flow looks consistentBringing
MilitaryAuditStatusintocomplete_military_affiliation_initializationand setting the provider’s top-levelmilitaryStatustoTENTATIVEwith a correspondingMILITARY_FILE_UPLOADProviderUpdateDatarecord matches the new audit lifecycle and uses the same transactional/batching pattern as other flows in this client.Also applies to: 674-779
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
257-261: Comment still says "declined" but should say "approved".The exception message on line 260 was corrected, but the comment on line 257 still says "Verify militaryStatus is set to declined" when the code checks for
'approved'.🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved':
🧹 Nitpick comments (3)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
110-117: Dead code after refactor tomax().Since
military_affiliationsis validated as non-empty on line 108,max()on line 111-112 will always return an element. The check on lines 114-117 is now unreachable dead code.🔎 Proposed cleanup
# Find the military affiliation record with the most recent dateOfUpload matching_military_affiliation = max( military_affiliations, key=lambda ma: datetime.fromisoformat(ma['dateOfUpload']) ) - if not matching_military_affiliation: - raise SmokeTestFailureException( - f'No military affiliation record found for today. Military affiliations: ({military_affiliations})' - ) if matching_military_affiliation['status'] != 'active':backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (2)
44-47: Remove redundant UUID conversion.Line 46 converts
provider_idto a string and back to a UUID, butprovider_idis already a UUID object from line 30 (validated by the schema). The conversion is unnecessary.🔎 Proposed simplification
provider_records = config.data_client.get_provider_user_records( compact=compact, - provider_id=UUID(str(provider_id)), + provider_id=provider_id, )
74-87: Consider simplifying redundant audit result checks.The code determines
event_typebased onaudit_result(lines 68-72), then checksaudit_resultagain to decide which notification method to call (lines 75-87). Consider checkingevent_typeinstead to avoid the redundancy:🔎 Proposed simplification
try: - if audit_result == MilitaryAuditStatus.APPROVED: + if event_type == EventType.MILITARY_AUDIT_APPROVED: logger.info('Sending military audit approved notification to provider') config.email_service_client.send_military_audit_approved_notification( compact=compact, provider_email=provider_email, ) else: logger.info('Sending military audit declined notification to provider') config.email_service_client.send_military_audit_declined_notification( compact=compact, provider_email=provider_email, audit_note=audit_note, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
✅ Files skipped from review due to trivial changes (2)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_bulk_upload.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/bulk_upload.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-30T17:30:31.252Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 864
File: backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py:866-866
Timestamp: 2025-06-30T17:30:31.252Z
Learning: In the CompactConnect project, the `sqs_handler` decorator transforms Lambda functions to accept `(event, context)` parameters and automatically handles iteration over `event['Records']`, parsing JSON from each record's body, and managing SQS batch item failures. Functions decorated with `sqs_handler` have their underlying implementation accept a single `message: dict` parameter, but the decorator makes them callable with standard Lambda `(event, context)` signature. Tests should call these decorated functions with `(event, context)` parameters.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧬 Code graph analysis (3)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
MilitaryAuditEventDetailSchema(89-96)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (7)
call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)SmokeTestFailureException(13-17)load_smoke_test_env(224-227)backend/compact-connect/tests/smoke/config.py (1)
load_smoke_test_env(100-103)
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_latest_military_affiliation_status(769-782)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestAndLintPurchases
🔇 Additional comments (10)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (6)
8-17: LGTM!The imports are appropriate and align with the new test functionality being added.
94-106: LGTM!The validation logic for tentative status is correct. The previous typo in the comment has been addressed.
159-170: LGTM!The validation for
notApplicablestatus follows the same consistent pattern as the tentative check above.
172-196: LGTM!Well-structured helper that encapsulates the audit endpoint call and provider data retrieval, reducing duplication in the test function while maintaining the linear narrative readability of the smoke test. Based on learnings, this approach aligns with project conventions.
226-274: LGTM!The test logic is well-structured:
- Proper try/finally pattern ensures cleanup of the test staff user even if assertions fail
- Tests both declined and approved audit flows with appropriate verification
- Clean separation between the two audit scenarios
277-281: LGTM!The new test is appropriately added to the main execution block, maintaining the logical test order: upload → patch → audit.
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (1)
67-72: No changes needed. Lines 70 and 75 correctly compareaudit_resultdirectly withMilitaryAuditStatusenum members. SinceCCEnuminherits fromStrEnum, these comparisons work correctly without.value.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (3)
13-13: LGTM: Import properly added.The
MilitaryAuditStatusimport is correctly placed and used later in the privilege purchase flow to determine military benefits eligibility.
353-358: LGTM: Military status determination logic is sound.The logic correctly determines active military status based on the provider-level audit outcome:
TENTATIVEandAPPROVEDstatuses grant military benefits (fee waivers, military-specific attestations)DECLINEDandNOT_APPLICABLEstatuses require standard (non-military) attestationsThe defensive null-handling on line 355 is good practice, ensuring consistent behavior even if upstream defaults are not set.
343-351: LGTM: Upload processing validation is correct.The check for
INITIALIZINGstatus correctly prevents privilege purchase when document processing has failed, providing clear user guidance for remediation. The existing test suite provides adequate coverage for this validation, including edge cases for missing military records, stuck INITIALIZING status, and successful uploads with various approval states.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
257-261: Fix copy-paste error in comment.The comment on line 257 says "Verify militaryStatus is set to declined" but the code checks for 'approved'. This was flagged in a previous review and marked as addressed, but the comment still needs correction.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved': raise SmokeTestFailureException( f'Military status is not approved. Status: {provider_data_after_approval.get("militaryStatus")}' )backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
986-990: Consider removing the unusedlatest_military_affiliationvariable or documenting its purpose.The
latest_military_affiliationvariable is only used for existence validation. If the intent is purely to verify at least one active affiliation exists, consider renaming or extracting the check to make intent clearer.This was flagged in a past review. If intentionally kept for potential future use or explicit validation semantics, a brief comment would help clarify.
🧹 Nitpick comments (2)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
172-196: LGTM!The helper function is well-structured with appropriate error handling and clear success messages. It effectively reduces duplication within the audit test.
Optional: Consider inlining per project smoke test patterns.
Based on project learnings, smoke tests typically maintain linear narrative readability by avoiding helper functions. However, this helper only reduces duplication within a single test and doesn't significantly impact readability, so this is purely optional.
Based on learnings, smoke tests in this project typically avoid helper functions for linear readability.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
748-763: Edge case: Multiple records with identicaldateOfUploadcould all be set to ACTIVE.If two military affiliation records have exactly the same
dateOfUploadtimestamp (e.g., created in rapid succession), the equality check at line 750 would match multiple records, setting all of them toACTIVEinstead of just one.Consider using object identity comparison or comparing on a unique field:
🔎 Proposed fix
# Update all military affiliation records for record in initializing_military_affiliation_records: - if record.dateOfUpload == latest_military_affiliation_record.dateOfUpload: + if record is latest_military_affiliation_record: record.update({'status': MilitaryAffiliationStatus.ACTIVE.value}) else: record.update({'status': MilitaryAffiliationStatus.INACTIVE.value})
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-08-25T19:04:48.800Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:288-293
Timestamp: 2025-08-25T19:04:48.800Z
Learning: In the CompactConnect project, landonshumway-ia has already provided feedback on patch cleanup patterns in test setup methods for the account recovery tests, so similar suggestions about stopping patches in setUp/tearDown should not be repeated.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestAndLintPurchases
🔇 Additional comments (12)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (7)
5-5: LGTM!The new imports are all utilized appropriately in the extended smoke tests.
Also applies to: 10-15
94-106: LGTM!The validation correctly ensures that after upload, the provider's military status is reset to 'tentative' with an empty status note.
110-113: LGTM!Using
max()withdatetime.fromisoformatas the key function is an effective way to find the most recent military affiliation by upload date.
159-170: LGTM!The validation correctly ensures that after marking all military affiliations as inactive, the provider's military status transitions to 'notApplicable' with an empty status note.
198-251: LGTM!The declined audit flow is well-structured with clear step-by-step verification. The test properly creates a staff user with admin privileges and validates both the status transition and the note content.
272-274: LGTM!Excellent use of a finally block to ensure the test staff user is cleaned up regardless of test success or failure.
281-281: LGTM!The new audit test is properly integrated into the smoke test execution flow.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
30-33: LGTM!The import of
MilitaryAuditStatusis properly added to support the new military audit functionality.
887-912: LGTM!The transaction structure correctly uses
Updatefor the provider record (atomic operation) andPutfor the update record. The expression properly removesmilitaryStatusandmilitaryStatusNotewhile settingdateOfUpdate.
1015-1048: LGTM - Transaction pattern correctly uses atomic Update.The implementation correctly uses
Updateoperation withConditionExpressionfor the provider record (lines 1020-1038), which prevents race conditions by atomically updating only the specific fields. This addresses the concern from the previous review.
822-822: LGTM!Good refactoring to use the existing
inactivate_current_military_affiliation_recordsmethod instead of inline logic.
847-848: Type mismatch:provider_idshould be converted toUUID.The
get_provider_user_recordsmethod expectsprovider_id: UUID(line 182), but here astris passed without conversion. Compare to line 684 whereUUID(provider_id)is correctly used.🔎 Proposed fix
# Get provider records - provider_user_records = self.get_provider_user_records(compact=compact, provider_id=provider_id) + provider_user_records = self.get_provider_user_records(compact=compact, provider_id=UUID(provider_id))⛔ Skipped due to learnings
Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 1135 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654 Timestamp: 2025-10-15T19:53:00.422Z Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.Learnt from: landonshumway-ia Repo: csg-org/CompactConnect PR: 796 File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64 Timestamp: 2025-06-03T16:41:07.757Z Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
257-257: Fix incorrect comment.The comment states "Verify militaryStatus is set to declined" but the subsequent assertion checks for 'approved'. This appears to be a copy-paste error that was not fully corrected.
🔎 Proposed fix
- # Verify militaryStatus is set to declined + # Verify militaryStatus is set to approved if provider_data_after_approval.get('militaryStatus') != 'approved':
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/docs/internal/api-specification/latest-oas30.jsonbackend/compact-connect/docs/internal/postman/postman-collection.jsonbackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-08-25T19:04:48.800Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:288-293
Timestamp: 2025-08-25T19:04:48.800Z
Learning: In the CompactConnect project, landonshumway-ia has already provided feedback on patch cleanup patterns in test setup methods for the account recovery tests, so similar suggestions about stopping patches in setUp/tearDown should not be repeated.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryAuditStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
get_military_affiliation_records(784-797)get_latest_military_affiliation(799-807)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCAwsServiceException(52-53)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
current_standard_datetime(275-279)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (3)
backend/compact-connect/tests/smoke/smoke_common.py (8)
SmokeTestFailureException(13-17)call_provider_users_me_endpoint(230-238)create_test_staff_user(107-134)delete_test_staff_user(86-104)get_api_base_url(196-197)get_provider_user_auth_headers_cached(168-176)get_staff_user_auth_headers(179-183)load_smoke_test_env(224-227)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/__init__.py (2)
compact(32-33)compact(172-173)backend/compact-connect/tests/smoke/config.py (1)
load_smoke_test_env(100-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (8)
backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (6)
5-5: LGTM!The new imports support the audit test workflow and datetime parsing for finding the most recent military affiliation record.
Also applies to: 10-12, 15-15
94-113: LGTM!The addition of
militaryStatusandmilitaryStatusNotevalidation after upload correctly verifies that the status is set to 'tentative' with an empty note. Usingmax()withdatetime.fromisoformatto find the most recent military affiliation is more robust than date-string comparison.
159-170: LGTM!The validation correctly verifies that when all military affiliations are set to inactive, the provider's
militaryStatusbecomes 'notApplicable' andmilitaryStatusNoteis cleared.
172-196: LGTM!The helper function appropriately encapsulates the audit endpoint call and provider data retrieval, reducing duplication in the test while maintaining clarity.
198-275: LGTM!The audit test is well-structured and comprehensive:
- Creates and cleans up test staff user properly with
try-finally- Tests both decline (with note) and approve (clearing note) workflows
- Validates provider data after each audit action
- Follows the project's smoke test pattern of keeping tests as single narrative functions for readability
Based on learnings, this structure is preferred for smoke tests in this project.
281-281: LGTM!The new audit test is appropriately added to the smoke test execution flow.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
30-30: LGTM—import supports new audit functionality.The
MilitaryAuditStatusimport is required for the newprocess_military_auditmethod and follows the existing import structure.
829-941: LGTM—well-structured military affiliation termination flow.The
end_military_affiliationmethod correctly:
- Removes
militaryStatusandmilitaryStatusNotefields usingREMOVEin theUpdateExpression- Creates a provider update record tracking the removal with
removedValues- Sets all
INITIALIZING/ACTIVEmilitary affiliation records toINACTIVE- Executes everything in batched DynamoDB transactions with proper error handling
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/README.md (1)
40-44: Consider specifying a minimum Python version."Python 3.x" is vague and may cause confusion since the project likely requires a specific minimum version (e.g., Python 3.11 or 3.12) due to language features or dependency requirements. Consider updating to specify the minimum supported version.
🔎 Suggested improvement
### 4. Python Dependencies Install the required Python packages. The smoke tests use the same dependencies as the main codebase. Ensure you have: -- Python 3.x +- Python 3.12 or higher (match the version used in project lambdas) - All dependencies from the project's requirements files
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/tests/smoke/README.mdbackend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-06-04T17:38:22.392Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 824
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:461-640
Timestamp: 2025-06-04T17:38:22.392Z
Learning: For smoke tests in backend/compact-connect/tests/smoke/, prioritize linear narrative readability over reducing complexity metrics. These tests are designed to be run manually by developers for verification, and should be readable from top to bottom like a story, allowing developers to follow the complete workflow without jumping between helper methods.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-07-22T03:36:17.137Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/purchases/requirements-dev.txt:15-0
Timestamp: 2025-07-22T03:36:17.137Z
Learning: In CompactConnect, requirements-dev.txt files for Lambda functions are used exclusively for running tests and development, not for actual Lambda runtime environments. Concerns about runtime compatibility (like OpenSSL versions) don't apply to these development dependency files.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-06-23T20:19:18.952Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 877
File: backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py:22-192
Timestamp: 2025-06-23T20:19:18.952Z
Learning: In the CompactConnect project, smoke test functions (like test_practitioner_email_update in backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py) should remain as single large functions rather than being broken down into smaller helper functions, based on the project maintainer's preference and reasoning.
Applied to files:
backend/compact-connect/tests/smoke/README.mdbackend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py
📚 Learning: 2025-06-23T21:31:15.787Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 870
File: backend/compact-connect/tests/smoke/license_deactivation_privilege_smoke_tests.py:128-132
Timestamp: 2025-06-23T21:31:15.787Z
Learning: For smoke tests in the CompactConnect project, broad exception handling (like `except Exception as e:`) is acceptable since these are not runtime code and prioritize simplicity over specific exception handling.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-08-22T16:02:55.832Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:142-161
Timestamp: 2025-08-22T16:02:55.832Z
Learning: In CompactConnect smoke tests, static emails for test staff users are intentionally used because the test framework can detect if a user already exists with that email and will reuse the existing user (who already has the needed permissions configured) rather than failing on username collisions.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-09-09T19:28:53.260Z
Learnt from: jsandoval81
Repo: csg-org/CompactConnect PR: 1071
File: webroot/tests/helpers/setup.ts:153-155
Timestamp: 2025-09-09T19:28:53.260Z
Learning: In the CompactConnect project, component tests are kept minimal and focus primarily on basic functionality. The main testing effort is concentrated on the data layer (models, store, etc.). For component tests, API stub methods only need to exist to prevent runtime errors - they don't need to return specific mock data since components aren't being tested for full API integration scenarios.
Applied to files:
backend/compact-connect/tests/smoke/README.md
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (2)
backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py (1)
176-188: LGTM!The logic correctly determines active military status by checking the
militaryStatusfield with an appropriate default of'notApplicable'. The attestation list modification is safe since'personal-information-home-state-attestation'is guaranteed to exist in the statically defined list above. The exclusion of'declined'status from the active military check is intentionally correct—declined military affiliations should not receive military attestation benefits.backend/compact-connect/tests/smoke/README.md (1)
1-149: Well-documented README for smoke tests.The documentation is comprehensive, covering all essential aspects: prerequisites, environment setup, execution instructions, troubleshooting, and contribution guidelines. The structure aligns well with the project's approach to smoke tests being run manually by developers for verification. Good security practice noted on line 81 regarding credential handling.
jusdino
left a comment
jusdino
left a comment
There was a problem hiding this comment.
Looks good! A few thoughts:
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/schema/data_event/api.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py
Outdated
Show resolved
Hide resolved
...ct-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
90-100: LGTM! Schema is correctly implemented.The
MilitaryAuditEventDetailSchemais well-structured with appropriate fields and validation. The use ofOneOfto restrictauditResultto onlyAPPROVEDandDECLINEDvalues from the broaderMilitaryStatusenum is functional and clear.Note: A past review comment asked about creating a dedicated
approved/declinedenum. While the current approach works correctly, if this validation list appears in multiple places, consider extracting it into a constant or dedicated enum to reduce duplication.
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (1)
87-169: Solid coverage of multi-affiliation activation, with minor determinism nitThe test accurately verifies that, when two initializing military affiliation records exist, the newer
dateOfUploadbecomesactiveand the older becomesinactive, matching the self‑healing behavior.If you want to make this even more deterministic, consider capturing a single
now = datetime.now(tz=UTC)and derivingolder_date/newer_datefrom it, rather than callingdatetime.nowtwice, to avoid any edge‑case drift.backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
784-808: New military affiliation accessors are consistent, with small reuse opportunity
get_military_affiliation_recordsandget_latest_military_affiliationfollow the same filtered‑list / sorted‑latest conventions as the other record helpers and will make the data client logic much clearer.You might consider reimplementing
get_latest_military_affiliation_statusin terms ofget_latest_military_affiliation()to avoid sorting twice in two different places, but that's strictly a cleanliness win.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
831-837: end_military_affiliation flow is coherent and matches business semantics
end_military_affiliationcleanly implements the “stop tracking military affiliation” behavior by:
- removing
militaryStatus/militaryStatusNotefrom the provider (and stampingdateOfUpdate),- creating a
MILITARY_AFFILIATION_ENDEDprovider update record withprevious+removedValues,- and setting all INITIALIZING/ACTIVE affiliation records to INACTIVE via transactional writes.
Using the new
get_military_affiliation_recordshelper with a status filter and re‑serializing each updated affiliation is consistent with existing CCDataClass patterns. This looks ready to support the “end affiliation” API surface.Also applies to: 838-949
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-10-28T15:05:49.315Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1698-1701
Timestamp: 2025-10-28T15:05:49.315Z
Learning: In the CompactConnect Python codebase, enums such as InvestigationStatusEnum, InvestigationAgainstEnum, ActiveInactiveStatus, LicenseEncumberedStatusEnum, and PrivilegeEncumberedStatusEnum derive from StrEnum. When writing DynamoDB AttributeValues (e.g., {'S': ...}) in files like backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py, pass the enum member directly (without .value). Future reviews should not recommend using .value for these enums.
Applied to files:
backend/compact-connect/lambdas/python/purchases/handlers/privileges.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
📚 Learning: 2025-10-15T19:53:00.422Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:2642-2654
Timestamp: 2025-10-15T19:53:00.422Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py and provider_record_util.py, the get_adverse_action_by_id method correctly handles UUID vs string comparison as written. Attempting to add explicit type conversion causes the mapping to fail. The current implementation should not be changed to add UUID/string conversion logic.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py
📚 Learning: 2025-06-03T16:41:07.757Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 796
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:58-64
Timestamp: 2025-06-03T16:41:07.757Z
Learning: The static `get_provider_record` method in `ProviderRecordUtility` is legacy code being phased out in favor of the `ProviderUserRecords` class. During this migration, they are keeping the static method backwards compatible (returning `None`) while the new `ProviderUserRecords` class implements the better pattern of raising exceptions.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
📚 Learning: 2025-05-13T21:20:51.133Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 798
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py:86-90
Timestamp: 2025-05-13T21:20:51.133Z
Learning: The implementation of `PositiveDecimal` in `cc_common/data_model/schema/fields.py` is intentionally kept simple without support for stacked validators. If more complex validation is needed, the preferred approach is to create another custom field class.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-11-13T21:02:07.064Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1188
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:187-198
Timestamp: 2025-11-13T21:02:07.064Z
Learning: In Python, StrEnum members inherit from str and can be directly used in operations that accept strings. For example, int(UpdateTierEnum.TIER_ONE) where TIER_ONE = '1' works without needing .value because the enum member itself is a string. This applies to type conversions and string operations on StrEnum members.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
🧬 Code graph analysis (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py (1)
ForgivingSchema(21-25)
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
get_latest_military_affiliation_status(769-782)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
MilitaryAffiliationStatus(4-7)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (3)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
put_default_provider_record_in_provider_table(477-497)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (1)
attestations(60-61)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py (2)
MilitaryAffiliationData(10-51)dateOfUpload(46-47)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (12)
backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (2)
13-13: LGTM - Import added for new military status handling.The MilitaryStatus enum import supports the updated logic that determines active military status from the top-level provider record.
343-360: LGTM - Military status logic correctly updated.The implementation properly handles the new military status model:
Upload status check (lines 343-351): Validates that the latest military affiliation upload succeeded (not INITIALIZING), preventing purchases when S3 document processing failed.
Active military determination (lines 353-357): Correctly reads the top-level
militaryStatusfield and treats TENTATIVE or APPROVED as active military. WhenmilitaryStatusis None (for records created before this field existed), it correctly evaluates to False (not active).Attestation validation (line 360): Properly passes the computed
user_active_militaryflag to validation, ensuring military users provide required military attestations.The logic correctly separates upload status validation from military status determination, and the inline comments clearly explain the intent.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (3)
303-328: LGTM - Test helper correctly updated for new military status model.The renamed helper function
_when_testing_military_statusproperly:
- Sets
militaryStatusdirectly on the provider record (line 317) instead of loading military affiliation records- Generates appropriate attestations based on
expected_military_parameter- Validates the purchase client receives the correct
user_active_militaryvalueThis aligns with the handler's updated logic that reads military status from the top-level provider record.
330-359: LGTM - Comprehensive test coverage for all military status values.The test suite now covers all four
MilitaryStatusenum values:
- TENTATIVE (line 335): Correctly validates active military = True
- APPROVED (line 343): Correctly validates active military = True
- DECLINED (line 351): Correctly validates active military = False
- NOT_APPLICABLE (line 359): Correctly validates active military = False (newly added)
The test method names clearly indicate which status is being tested, and each test properly uses the enum values. This ensures the handler correctly determines
user_active_militarybased on the provider'smilitaryStatus.
974-984: LGTM - Attestation validation test updated for new status handling.The test correctly:
- Updates the docstring to reflect that TENTATIVE or APPROVED are considered active military status (line 974)
- Sets
militaryStatus: 'approved'on the provider record (lines 982-984) instead of loading military affiliation records- Validates that military attestations are required when the user has active military status
This aligns with the handler's updated military status determination logic.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
3-3: LGTM! Imports are necessary and correctly added.The
MilitaryStatusandOneOfimports are properly used in the newMilitaryAuditEventDetailSchemafor validating audit results.Also applies to: 9-9
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
15-18: MilitaryStatusField matches existing status field patternsImporting
MilitaryStatusand wiringMilitaryStatusFieldwithOneOf([entry.value for entry in MilitaryStatus])is consistent with the other enum-backed fields here and should integrate cleanly with the schemas that consume it.Also applies to: 136-139
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (2)
35-45: Good setup of provider state and S3 key format for military affiliation flowSeeding the provider with
DECLINED+ note before invoking the POST and switching the mock S3 key to a full ISO timestamp (including time + offset) keeps these tests aligned with the new sort key / document key contract and the provider-update expectations.Also applies to: 52-59
170-232: Tests for tentative provider status and provider update record look comprehensiveThe new tests validating that:
- the provider record is updated to
tentativewith an empty note, and- exactly one
MILITARY_FILE_UPLOADprovider update record is created with the expected previous/updated military status fieldsprovide good end‑to‑end verification of the new initialization behavior and history tracking.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
17-28: Provider schemas correctly extended for military status and historyAdding
MilitaryStatusFieldtoProviderRecordSchemaand mirroringmilitaryStatus/militaryStatusNoteonProviderUpdatePreviousRecordSchemafits the existing schema patterns and ensures both the live record and historical snapshots can carry military audit state.Also applies to: 80-83, 213-214
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
958-963: Defaulting militaryStatus and note in API response is appropriateEnsuring
provider['militaryStatus']defaults toMilitaryStatus.NOT_APPLICABLE.valueandprovider['militaryStatusNote']to''when absent gives clients a predictable shape across old and new records without mutating stored data.backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
966-1063: process_military_audit implementation aligns with split audit vs record status designThe
process_military_auditmethod:
- validates there is at least one military affiliation via
get_latest_military_affiliation(),- captures the provider's previous state,
- creates a
MILITARY_AUDITProviderUpdateDatarecord carryingmilitaryStatusandmilitaryStatusNote,- and updates the provider's top‑level
militaryStatus/ note /dateOfUpdatein a single DynamoDB transaction, with aConditionExpressionto ensure the record exists.Passing the
MilitaryStatusenum member directly into the Dynamo AttributeValue ({'S': military_status}) is consistent with the StrEnum pattern used throughout this file (e.g., lines 733, 1462). SinceMilitaryStatusis aCCEnumsubclass andCCEnumextendsStrEnum, this pattern works correctly.Docstring and behavior correctly describe provider‑only updates (no affiliation mutation), which matches the current data model.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (1)
90-99: Well-structured schema with optional optimization opportunity.The schema correctly models military audit event details. The validation pattern ensures type safety by constraining
auditResultto enum values.Optional: Extract OneOf validator to module-level constant
For improved readability and to avoid repeated list comprehension, consider extracting the validation list:
+_MILITARY_AUDIT_STATUS_VALUES = [entry.value for entry in MilitaryAuditStatus] + class MilitaryAuditEventDetailSchema(ForgivingSchema): """Schema for military audit event details.""" compact = Compact(required=True, allow_none=False) providerId = UUID(required=True, allow_none=False) auditResult = String( - required=True, allow_none=False, validate=OneOf([entry.value for entry in MilitaryAuditStatus]) + required=True, allow_none=False, validate=OneOf(_MILITARY_AUDIT_STATUS_VALUES) ) auditNote = String(required=False, allow_none=False) eventTime = DateTime(required=True, allow_none=False)backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
60-74: Data update and event publishing work correctly.The handler appropriately:
- Updates provider and military affiliation records via
data_client.process_military_audit- Publishes a notification event via
event_bus_client.publish_military_audit_eventNote: If the event publish (line 68) fails after the data update (line 60) succeeds, the provider record will be updated but no notification event will be sent. This may be acceptable if your system design assumes eventual consistency or has retry mechanisms elsewhere.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (1)
194-194: Update outdated comment to reflect new military status model.The comment references "empty list of military affiliations," but the code now checks the provider-level
militaryStatusfield instead of military affiliation lists.🔎 Suggested comment update
- # in this test, the user had an empty list of military affiliations, so this should be false + # in this test, the user has no militaryStatus set (defaults to None/notApplicable), so this should be falsebackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (1)
81-83: Add length validation to militaryStatusNote for consistency.The
militaryStatusNotefield lacks a length constraint, while similar text fields in this schema usevalidate=Length(min, max)(e.g.,givenName,middleName,familyName). Adding a reasonable max length would align with the pattern used for other optional note/text fields:Proposed change
# Military audit status fields militaryStatus = MilitaryStatusField(required=False, allow_none=False) -militaryStatusNote = String(required=False, allow_none=False) +militaryStatusNote = String(required=False, allow_none=False, validate=Length(max=500))Note: The
militaryStatusdefault behavior is already handled correctly inbackend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(lines 959–960), which initializes it toMilitaryStatus.NOT_APPLICABLE.valuewhen absent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.pybackend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
📚 Learning: 2025-04-29T15:30:08.849Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/__init__.py:17-23
Timestamp: 2025-04-29T15:30:08.849Z
Learning: The MilitaryAffiliationData class in CompactConnect is explicitly designed with read-only properties as stated in its docstring, even though it inherits from CCDataClass which supports initialization without data followed by using setters.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py
📚 Learning: 2025-07-08T18:40:24.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 882
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py:287-359
Timestamp: 2025-07-08T18:40:24.408Z
Learning: In the CompactConnect codebase, landonshumway-ia prefers to avoid extraneous unit tests when existing test coverage is already sufficient to catch bugs. For the get_privilege_purchase_options method's live-jurisdiction filtering logic, the existing tests in the purchases test suite provide adequate coverage without needing additional edge case tests.
Applied to files:
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
🧬 Code graph analysis (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
MilitaryStatusField(136-138)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCEnum(211-221)
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (3)
backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
put_default_provider_record_in_provider_table(477-497)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)backend/compact-connect/lambdas/python/purchases/handlers/privileges.py (1)
post_purchase_privileges(177-470)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (14)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/common.py (1)
15-20: LGTM! Clean enum addition.The
MilitaryAuditStatusenum follows the established pattern in this file and provides clear semantics for military documentation audit outcomes.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py (2)
7-7: LGTM! Import added for enum validation.The
MilitaryAuditStatusimport is used correctly in the schema below for validating audit results.
9-9: LGTM! OneOf validator import.The
OneOfimport is appropriately used to constrainauditResultvalues to the defined enum members.backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (5)
14-27: LGTM! Well-documented handler with appropriate authorization.The function signature, decorators, and docstring clearly define the handler's purpose and security requirements. The
@authorize_compact_level_only_action(action=CCPermissionsAction.ADMIN)ensures only compact admins can audit military documentation.
28-38: LGTM! Proper context setup and parameter extraction.The code correctly:
- Uses logger context for request tracing
- Extracts and validates path parameters with
to_uuidfor type safety- Captures
cognito_subfor audit trail purposes
40-46: LGTM! Robust error handling for request parsing.The try-except blocks appropriately handle both JSON decode errors and schema validation errors, providing clear error messages to the caller via
CCInvalidRequestException.
48-57: LGTM! Safe enum conversion with appropriate logging.The conversion from string to
MilitaryAuditStatusenum at line 52 is safe becauseMilitaryAuditRequestSchemavalidation on line 42 ensures the string value is valid. The logging captures the status but appropriately excludesmilitary_status_notewhich may contain sensitive information.
76-78: LGTM! Appropriate success response.The handler logs completion and returns a success message. While the response could include details about the updated resource, the minimal payload is acceptable for this use case.
backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (4)
303-328: LGTM! Clean refactoring of military status helper.The helper method correctly transitions from loading military affiliation records to setting the provider-level
militaryStatusfield. The logic properly verifies that the handler interprets the status as expected.
330-363: Excellent test coverage for all military status values.All four
MilitaryStatusenum values are tested with clear expectations:
TENTATIVEandAPPROVEDcorrectly treated as active militaryDECLINEDandNOT_APPLICABLEcorrectly treated as not active militaryThe tests are well-documented and align with the handler implementation.
977-1012: LGTM! Military attestation validation correctly updated.The test properly validates that military attestations are required when
militaryStatusis set to'approved'(an active military status). The docstring accurately reflects that tentative or approved statuses are considered active military, and the test uses the provider-levelmilitaryStatusfield consistently with the refactoring.
366-387: No issues found. This test is correct and should be kept.The test validates an active code path in the handler that checks
get_latest_military_affiliation_status()and raises an exception when the status isINITIALIZING(indicating a document upload that failed S3 processing). The'initializing'status is a legitimate value in theMilitaryAffiliationStatusenum and is actively used throughout the codebase. The error message matches the handler's exact exception message at lines 348-350 ofprivileges.py.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
25-25: LGTM!The import is correctly placed and necessary for the new military status fields.
214-215: LGTM!The military status fields are correctly added to the provider update history schema, maintaining consistency with the main
ProviderRecordSchema. This ensures the audit trail properly captures changes to military status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (3)
11-12: Consider adding return type hint.The function signature is clear, but adding
-> Nonewould improve type safety.🔎 Proposed refactor
-def military_audit_notification_listener(message: dict, tracker: NotificationTracker): +def military_audit_notification_listener(message: dict, tracker: NotificationTracker) -> None:
43-61: LGTM with minor optimization opportunity.The provider record retrieval and email validation logic is solid. Error handling appropriately logs and re-raises exceptions.
Optional optimization: Line 46's
UUID(str(provider_id))conversion may be redundant ifprovider_idis already a UUID object from the schema. Consider checking the schema's type forproviderIdand simplifying if it already returns UUID:🔎 Potential refactor
provider_records = config.data_client.get_provider_user_records( compact=compact, - provider_id=UUID(str(provider_id)), + provider_id=provider_id if isinstance(provider_id, UUID) else UUID(str(provider_id)),Or if the schema guarantees UUID type, simply:
- provider_id=UUID(str(provider_id)), + provider_id=provider_id,
62-65: Consider checking idempotency earlier for efficiency.The idempotency check correctly prevents duplicate notifications. However, it occurs after retrieving provider records (line 44). If the notification was already sent, this results in an unnecessary database call.
Optional optimization: Move the idempotency check before the provider record retrieval to avoid unnecessary database calls:
🔎 Potential refactor
with logger.append_context_keys( compact=compact, provider_id=str(provider_id), audit_result=audit_result, ): logger.info('Processing military audit notification') + # Check if we should send the notification (idempotency) + if not tracker.should_send_provider_notification(): + logger.info('Skipping provider notification (already sent successfully)') + return + # Get provider records to find registered email try: provider_records = config.data_client.get_provider_user_records( compact=compact, provider_id=UUID(str(provider_id)), ) provider_record = provider_records.get_provider_record() except Exception as e: logger.error('Failed to retrieve provider records for notification', exception=str(e)) raise provider_email = provider_record.compactConnectRegisteredEmailAddress if not provider_email: # this should not be possible, since only registered providers can upload military documentation # log the error and raise an exception message = 'Provider registered email not found in system' logger.error(message) raise CCInternalException(message) - # Check if we should send the notification (idempotency) - if not tracker.should_send_provider_notification(): - logger.info('Skipping provider notification (already sent successfully)') - returnNote: Keep the current order if you need to verify the provider still exists before checking idempotency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/data-events/handlers/military_audit_events.py (3)
1-8: LGTM!Imports are well-organized and all necessary dependencies are present.
67-105: LGTM!The notification sending logic is well-implemented:
- Properly uses
MilitaryAuditStatusenum for type-safe comparisons- Correctly branches to send approved vs. declined notifications
- Includes audit notes only for declined notifications, which is appropriate
- Comprehensive error handling that logs failures, records them in the tracker, and re-raises to enable Lambda retries
- Idempotent notification tracking prevents duplicate sends
33-33: No action needed. TheeventTimefield inMilitaryAuditEventDetailSchemais correctly defined as a marshmallowDateTimefield, which deserializes ISO string inputs to Pythondatetimeobjects during validation. Calling.isoformat()on this datetime object is the correct approach.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (1)
433-498: LGTM!Comprehensive test coverage for the military affiliation ending workflow:
test_patch_provider_military_affiliation_removes_military_status_fieldscorrectly validates thatmilitaryStatusresets toNOT_APPLICABLEandmilitaryStatusNoteis cleared.test_patch_provider_military_affiliation_creates_provider_update_recordvalidates the provider update record with correctupdateType,previousstate, andremovedValues.One minor observation: the assertion at line 496 (
self.assertEqual(['militaryStatus', 'militaryStatusNote'], update_data.removedValues)) is order-sensitive. If the order ofremovedValuesis not guaranteed by the implementation, consider usingself.assertCountEqualinstead to avoid potential flakiness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-response.jsonbackend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.jsonbackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.pybackend/compact-connect/lambdas/python/search/tests/function/test_populate_provider_documents.pybackend/compact-connect/lambdas/python/search/tests/function/test_provider_update_ingest.pybackend/compact-connect/lambdas/python/search/tests/function/test_search_providers.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
📚 Learning: 2025-04-29T01:57:43.684Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.jsonbackend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.jsonbackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/search/tests/function/test_populate_provider_documents.pybackend/compact-connect/lambdas/python/common/tests/resources/api/provider-response.jsonbackend/compact-connect/lambdas/python/search/tests/function/test_provider_update_ingest.py
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.jsonbackend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.jsonbackend/compact-connect/lambdas/python/common/tests/resources/api/provider-response.json
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.pybackend/compact-connect/lambdas/python/search/tests/function/test_search_providers.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/stacks/api_stack/v1_api/api_model.pybackend/compact-connect/lambdas/python/search/tests/function/test_populate_provider_documents.pybackend/compact-connect/lambdas/python/search/tests/function/test_provider_update_ingest.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-12-01T17:57:35.454Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/tests/app/test_search_persistent_stack.py:104-130
Timestamp: 2025-12-01T17:57:35.454Z
Learning: In backend/compact-connect/tests/app/test_search_persistent_stack.py, the OpenSearch Domain's EncryptionAtRestOptions.KmsKeyId assertion correctly uses {"Ref": encryption_key_logical_id} format. This pattern has been validated to work with CDK's opensearchservice.Domain construct.
Applied to files:
backend/compact-connect/lambdas/python/search/tests/function/test_search_providers.py
📚 Learning: 2025-08-21T19:57:13.276Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1014
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_account_recovery.py:0-0
Timestamp: 2025-08-21T19:57:13.276Z
Learning: In the CompactConnect codebase, ProviderData.recoveryExpiry property returns `datetime | None` after the type hint fix. The marshmallow DateTime field in the record schema handles conversion between ISO strings (database storage) and datetime objects (application use), so test assertions can directly compare datetime objects.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-04-29T21:14:36.205Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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))`.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-04T17:55:02.692Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1058
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:785-786
Timestamp: 2025-09-04T17:55:02.692Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the sanitize_provider_account_profile_fields_from_response method is called within generate_api_response_object, which serves as the central method that all endpoints use to construct provider response objects. This ensures that sensitive profile fields are sanitized at the source, eliminating the need for additional sanitization calls in downstream methods like those in utils.py.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
📚 Learning: 2025-09-03T22:27:56.621Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py:71-71
Timestamp: 2025-09-03T22:27:56.621Z
Learning: The privilegeJurisdictions field in provider schemas consistently uses load_default=set() pattern across multiple schema files in the CompactConnect codebase (provider/record.py and provider/api.py), and developers prioritize maintaining this consistency.
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
Repo: csg-org/CompactConnect PR: 1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
📚 Learning: 2025-08-14T20:56:56.938Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:5698-5700
Timestamp: 2025-08-14T20:56:56.938Z
Learning: In the CompactConnect project, there is a deliberate design pattern for API schemas: request schemas should include enum validation constraints (like encumbranceType with its allowed values), while response schemas should remain unconstrained (just "type": "string") to allow flexibility in API responses.
Applied to files:
backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
📚 Learning: 2025-10-15T19:56:58.050Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1135
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py:60-0
Timestamp: 2025-10-15T19:56:58.050Z
Learning: In the CompactConnect codebase, when migrating from a single field to a list field (e.g., clinicalPrivilegeActionCategory to clinicalPrivilegeActionCategories), both fields are intentionally kept as optional during the migration period to support backwards compatibility. Mutual exclusivity validation is not enforced during this phase, as the deprecated field will be removed in a follow-up PR with migration scripts.
Applied to files:
backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
📚 Learning: 2025-06-10T03:16:16.896Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 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.
Applied to files:
backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
🧬 Code graph analysis (4)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (1)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
pop(77-78)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
MilitaryStatusField(136-138)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (1)
required(17-18)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
MilitaryStatus(296-302)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/fields.py (1)
MilitaryStatusField(136-138)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (2)
militaryStatus(151-158)militaryStatusNote(161-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (21)
backend/compact-connect/lambdas/python/search/tests/function/test_provider_update_ingest.py (1)
148-148: LGTM!The addition of
militaryStatus: 'notApplicable'correctly extends the expected OpenSearch document to include the new required field with its default value, matching the data model changes introduced in this PR.backend/compact-connect/lambdas/python/search/tests/function/test_populate_provider_documents.py (1)
113-113: Test expectation correctly updated for new required field.The addition of
'militaryStatus': 'notApplicable'appropriately updates the test expectations to match the new provider schema with the required militaryStatus field. The enum value is valid per the OpenAPI schema definition.backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
1567-1576: LGTM! Military status fields properly added to provider responses.The
militaryStatusandmilitaryStatusNotefields are correctly added to_common_provider_properties, which propagates them to the private provider response models. The enum values appropriately cover the full lifecycle (notApplicable, tentative, approved, declined), and making these fields optional in responses ensures backwards compatibility with existing provider records.
2896-2921: LGTM! Military audit request model is well-structured.The new
patch_military_audit_request_modelis properly implemented with:
- Appropriate enum subset
['approved', 'declined']for admin audit actions (vs. the broader provider-level status enum)- Correctly makes
militaryStatusrequired while keepingmilitaryStatusNoteoptional- Consistent
max_lengthconstraint (5000) with the provider response field- Proper security with
additional_properties=FalseNote: The past review comment concern has been addressed—
militaryStatusandmilitaryStatusNoteare now present in the provider response models via_common_provider_properties(lines 1567-1576).backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json (1)
14-14: LGTM!The
militaryStatusfield is correctly added with the default value"notApplicable", consistent with the new MilitaryStatus enum and provider data model changes.backend/compact-connect/lambdas/python/search/tests/function/test_search_providers.py (1)
286-286: LGTM!The test correctly expects
militaryStatus: 'notApplicable'in the sanitized output. Since the mock hit_sourcedoesn't includemilitaryStatus, this validates that the schema applies the default value during serialization—consistent with the provider data model changes.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_registration.py (1)
842-842: LGTM!The test correctly expects
militaryStatus: 'notApplicable'in thepreviousstate of the provider update record, reflecting that the provider data model now includes this field with its default value.backend/compact-connect/lambdas/python/common/tests/resources/api/provider-response.json (1)
14-14: LGTM!The
militaryStatusfield is correctly added to the provider response fixture with the default value, aligning with the updated provider API schema.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.py (2)
239-239: LGTM!The S3 key path now includes the full ISO8601 timestamp (including time and timezone offset), consistent with the PR's changes to track full upload datetime in the military affiliation sort key and S3 prefix.
339-354: LGTM!Good refactoring to extract
_generate_path_api_eventas a reusable helper, improving test maintainability and reducing code duplication.backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json (1)
284-299: LGTM!The schema correctly adds:
militaryStatuswith the appropriate enum values matching the MilitaryStatus data modelmilitaryStatusNotewith a sensible maxLength constraintBoth fields are appropriately not added to the
requiredarray since they have defaults or are optional.backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_public_lookup.py (2)
47-47: LGTM!Correctly validates that
militaryStatusis filtered from public query responses, ensuring sensitive military information is not exposed to unauthenticated callers.
458-459: LGTM!Correctly validates that both
militaryStatusandmilitaryStatusNoteare filtered from public provider detail responses, maintaining appropriate privacy controls for military-related data.backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json (1)
2280-2295: LGTM!The schema correctly adds
militaryStatusandmilitaryStatusNotewith definitions consistent with the QUERY_PROVIDERS_RESPONSE_SCHEMA. These fields are appropriately included in the detailed provider response schema (for authenticated users) while being excluded from public endpoints.backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json (1)
2280-2295: LGTM! Military status fields properly defined.The
militaryStatusenum field with values ["notApplicable", "tentative", "approved", "declined"] and the optionalmilitaryStatusNotefield (maxLength 5000) are well-defined and consistent with the MilitaryStatus enum. While both fields are marked as optional in this schema, themilitaryStatusfield will always be present in responses due to theload_default=MilitaryStatus.NOT_APPLICABLEin the record schema, ensuring backward compatibility.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (3)
16-16: LGTM! Imports properly added.The
MilitaryStatusenum andMilitaryStatusFieldvalidator imports are correctly placed and necessary for the new military status fields.Also applies to: 26-26
82-84: LGTM! Military status fields properly configured with sensible defaults.The
militaryStatusfield withload_default=MilitaryStatus.NOT_APPLICABLEensures backward compatibility with existing provider records that don't have military affiliation data. The field configuration withrequired=Falseandallow_none=Falseis appropriate for an optional field with a default value.The
militaryStatusNotefield is correctly optional. Note that while the JSON schema snapshot specifiesmaxLength: 5000, there's no explicitLengthvalidator here. This is consistent with some other String fields in this schema, suggesting length validation may be enforced at the API layer or via other mechanisms.
215-216: LGTM! Historical record schema correctly configured.The military status fields are appropriately added to
ProviderUpdatePreviousRecordSchemawithoutload_defaultvalues, preserving the actual historical state of provider records. This is the correct pattern for snapshot schemas.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (3)
16-16: LGTM! Import properly added.The
MilitaryStatusFieldimport is correctly placed in the fields import group.
131-133: LGTM! Military status fields properly configured for readPrivate responses.The fields are correctly defined with
militaryStatusasrequired=True(addressing the past review feedback) andmilitaryStatusNoteas optional. Therequired=TrueformilitaryStatusis safe because theload_default=MilitaryStatus.NOT_APPLICABLEin the record schema ensures this field is always present when serializing provider data to API responses.Based on past review feedback, the required=True setting has been properly applied.
187-188: LGTM! Military status field correctly restricted for general responses.The
militaryStatusfield is appropriately included asrequired=True, whilemilitaryStatusNoteis correctly excluded from the general response schema. The comment accurately documents this privacy distinction, ensuring that potentially sensitive audit notes are only available to users with readPrivate permissions.
Also addressed discrepancy for query URL in state API docs
landonshumway-ia
force-pushed
the
feat/military-audit-status
branch
from
4b9c005 to
8c1fb67
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/compact-connect/docs/internal/api-specification/latest-oas30.json`:
- Around line 9502-9506: Remove the hard-coded enum arrays from the
militaryStatus property in the response schemas; locate the militaryStatus
definitions used by the SandboLicenJlHz6gimzgVV and SandboLicenDTjDt3roB2dM
response schemas (responses for GET /provider-users/me, GET
/providers/{providerId}, and POST /providers/query) and delete the "enum": [...]
entries, leaving "type": "string" and the existing "description" intact so
responses accept any string values.
In
`@backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py`:
- Around line 17-27: The docstring for the military audit handler incorrectly
states it updates the latest military affiliation record; update the wording to
reflect current behavior: it only updates the provider record and creates a
provider_update via process_military_audit(). Edit the function/class docstring
and any inline comments (including the note around lines referencing
process_military_audit) to state that the audit result is persisted to the
provider record and a provider_update is created, removing or rephrasing any
claim about modifying military affiliation records.
- Around line 39-47: The code currently calls json.loads(event['body']) which
raises TypeError when event['body'] is None and results in a 500; update the
parsing block in military_audit.py to first check for a missing or null body
(e.g., body_text = event.get('body'); if not body_text: raise
CCInvalidRequestException('Missing request body') ) and then call
json.loads(body_text), and also add a specific except TypeError as e (or broaden
the except to catch TypeError) to raise CCInvalidRequestException with a
400-friendly message; keep the existing except ValidationError handling for
MilitaryAuditRequestSchema().load.
In
`@backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py`:
- Around line 21-46: The helper _when_testing_military_audit currently skips
adding militaryStatusNote when the value is falsy; change it to accept explicit
empty-string and explicit None by introducing a sentinel default (e.g., NO_NOTE
= object()) as the default for the military_status_note parameter and then add
militaryStatusNote to body whenever military_status_note is not the sentinel;
this preserves omitting the field when the caller truly omitted the argument
while allowing callers to pass "" or None (which json.dumps will render as "" or
null) for tests that need those explicit values.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
923-935: AvoidPut-overwriting affiliation records inend_military_affiliation().Using
Putto “update” each military affiliation record risks overwriting fields added later (or accidentally recreating items if keys drift). PreferUpdatewith aConditionExpressionlike the other flows in this file.Sketch of safer Update-based transaction item
- record.update({'status': MilitaryAffiliationStatus.INACTIVE.value}) - serialized_record = record.serialize_to_database_record() transaction_items.append( { - 'Put': { + 'Update': { 'TableName': self.config.provider_table_name, - 'Item': TypeSerializer().serialize(serialized_record)['M'], + 'Key': { + 'pk': {'S': record.pk}, + 'sk': {'S': record.sk}, + }, + 'UpdateExpression': 'SET `#status` = :status, dateOfUpdate = :dateOfUpdate', + 'ExpressionAttributeNames': {'#status': 'status'}, + 'ExpressionAttributeValues': { + ':status': {'S': MilitaryAffiliationStatus.INACTIVE}, + ':dateOfUpdate': {'S': self.config.current_standard_datetime.isoformat()}, + }, + 'ConditionExpression': 'attribute_exists(pk)', } } )
🧹 Nitpick comments (10)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
1565-1625: Consider adding a test case for missingauditNotein declined notification.The declined notification tests verify the happy path with an audit note and the missing recipients error case. However, the implementation in
email-notification-service.tshandles the case whereauditNoteis missing or empty (line 549). Consider adding a test to verify the email is sent correctly without the audit note message.📝 Suggested test case
it('should successfully send military audit declined notification email without audit note', async () => { const eventWithoutNote: EmailNotificationEvent = { ...SAMPLE_MILITARY_AUDIT_DECLINED_NOTIFICATION_EVENT, templateVariables: {} }; const response = await lambda.handler(eventWithoutNote, {} as any); expect(response).toEqual({ message: 'Email message sent' }); const emailCall = mockSESClient.commandCalls(SendEmailCommand)[0]; const htmlContent = emailCall.args[0].input.Content?.Simple?.Body?.Html?.Data; expect(htmlContent).toBeDefined(); expect(htmlContent).toContain('This message is to notify you that your military status documentation has been reviewed and declined by the compact staff.'); expect(htmlContent).not.toContain('Message from the compact staff:'); });backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
501-560: Thecompactparameter in both methods is used only for logging context and follows the established pattern in this class.Verified that
compactis used only for logging in these methods, consistent with similar methods likesendMultipleRegistrationAttemptNotificationEmailandsendPrivilegeDeactivationProviderNotificationEmail. While the parameter doesn't drive recipient lookup or template customization, this logging pattern is intentional across the service for tracking which compact a notification relates to.backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
788-796: Consider extracting shared sorting logic.This method duplicates the sorting logic from
get_latest_military_affiliation_status(lines 767-769). You could refactor to haveget_latest_military_affiliation_statuscall this method:♻️ Optional refactor
def get_latest_military_affiliation_status(self) -> str | None: """ Determine the provider's latest military affiliation status if present. :return: The military affiliation status of the provider if present, else None """ - if not self._military_affiliation_records: + latest = self.get_latest_military_affiliation() + if not latest: return None - - # we only need to check the most recent military affiliation record - latest_military_affiliation = sorted( - self._military_affiliation_records, key=lambda x: x.dateOfUpload, reverse=True - )[0] - - return latest_military_affiliation.status + return latest.statusbackend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py (1)
170-170: Minor: Redundant variable assignment.The
compactvariable is already assigned at line 142 and hasn't been modified since. This line at 170 is redundant.🔧 Suggested fix
- compact = original_provider_data.get('compact')backend/compact-connect/tests/smoke/military_affiliation_smoke_tests.py (1)
172-196: Consider inlining the new helper to preserve smoke-test “story” readability.
_call_audit_endpoint_and_get_provider_data(...)is fine mechanically, but these smoke tests are typically easiest to follow when the full flow is linear in one function. If maintainers still prefer that style, consider inlining it intotest_military_affiliation_audit()(or keeping it immediately adjacent to the call site with minimal branching). Based on learnings, smoke tests prioritize narrative readability.Also applies to: 198-275
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_s3_events.py (1)
87-169: Stabilize time-based assertions to avoid test flake.This test uses multiple
datetime.now(tz=UTC)calls (older/newer dates + key), then asserts exactisoformat()string matches. Consider using fixed datetimes (or onebase = datetime(..., tz=UTC)and derive from it) to make failures easier to reason about.backend/compact-connect/lambdas/python/data-events/tests/function/test_military_audit_events.py (1)
91-111: LGTM! Failure handling test ensures proper batch item failure recording.The test correctly verifies that when the email service fails, the handler records a batch item failure, enabling SQS to retry or route to DLQ.
Consider adding edge case tests (optional):
- Test behavior when provider email is missing (should raise
CCInternalException).- Test idempotency check (when
tracker.should_send_provider_notification()returnsFalse).backend/compact-connect/docs/internal/api-specification/latest-oas30.json (1)
6561-6582: Consider enforcingmilitaryStatusNotewhenmilitaryStatus=declined(spec or server-side).Right now the note is optional for both outcomes; if product expects “declined requires a note”, capture it via
oneOfin the request model or validate in the handler.backend/compact-connect/stacks/api_lambda_stack/provider_management.py (1)
375-406: Lambda permissions/nag suppression pattern matches existing handlers.RW on provider table + put-events to event bus aligns with
process_military_audit(...)+publish_military_audit_event(...)usage.Optional least-privilege follow-up
If the handler never reads from
self.persistent_stack.staff_users.user_table, you could drop that grant to tighten IAM.backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py (1)
977-988: PreferMilitaryStatus.APPROVEDover raw string for consistency.Proposed tweak
- # Set militaryStatus to 'approved' on the provider record + # Set militaryStatus to APPROVED on the provider record self.test_data_generator.put_default_provider_record_in_provider_table( - value_overrides={'militaryStatus': 'approved', 'providerId': TEST_PROVIDER_ID} + value_overrides={'militaryStatus': MilitaryStatus.APPROVED, 'providerId': TEST_PROVIDER_ID} )
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
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/provider-data-v1/handlers/military_audit.py
Show resolved
Hide resolved
...-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_military_audit.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py`:
- Around line 17-26: Fix the minor grammatical error in the module docstring in
military_audit.py: change the phrase "stored the provider record" to "stored in
the provider record" so the sentence reads that the audit result is stored in
the provider record; update the docstring where the endpoint description appears
(the top-level docstring for the military audit handler) accordingly.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (1)
58-64: Inaccurate inline comment.The comment "Update provider and military affiliation records" may not reflect the actual behavior if
process_military_auditonly updates the provider record. Consider aligning the comment with implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1233
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py:39-47
Timestamp: 2026-01-14T16:55:31.926Z
Learning: For the military audit endpoint at backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py, API Gateway validators are configured to guarantee that the request body exists and is non-empty, so defensive checks like event.get('body') or checking for None are not needed in the handler.
📚 Learning: 2026-01-14T16:55:31.926Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1233
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py:39-47
Timestamp: 2026-01-14T16:55:31.926Z
Learning: In backend/compact-connect/lambdas/python/provider-data-v1/handlers/*.py, rely on API Gateway validators to ensure the request body exists and is non-empty. Omit redundant defensive checks like event.get('body') or checking for None in handlers, and instead validate the body content only if gateway validation is insufficient for specific fields.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-12-16T21:43:07.408Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1219
File: backend/compact-connect/lambdas/python/search/handlers/search.py:131-140
Timestamp: 2025-12-16T21:43:07.408Z
Learning: In backend/compact-connect/lambdas/python/search/handlers/search.py, avoid logging the full request body. Do not log sensitive content by default. If logging is required for security investigations, redact or mask sensitive fields (e.g., credentials, tokens, PII) and log only safe metadata (method, path, status, user identifier). Use a secure, access-controlled audit log or feature flag to enable such logs, ensuring minimal exposure and compliance with security policies. This guideline targets Python backend handlers handling external requests and should be considered for similar files with request processing.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-08-22T21:04:46.552Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1029
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py:0-0
Timestamp: 2025-08-22T21:04:46.552Z
Learning: API Gateway always includes the 'body' key in Lambda events, even when the request body is empty/null. When event['body'] is None, json.loads(None) raises TypeError, not a missing key error.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-10-24T21:40:48.912Z
Learnt from: jusdino
Repo: csg-org/CompactConnect PR: 1167
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/investigation.py:59-66
Timestamp: 2025-10-24T21:40:48.912Z
Learning: For the investigation endpoints in backend/compact-connect/lambdas/python/provider-data-v1/handlers/investigation.py, API Gateway validators are configured to guarantee that the request body exists and is non-empty, so defensive checks like event.get('body') or '{}' are not needed in _load_investigation_patch_body.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2026-01-07T21:20:12.524Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1243
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:386-387
Timestamp: 2026-01-07T21:20:12.524Z
Learning: In backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py, the field clinicalPrivilegeActionCategories is required for encumbrance events in encumbranceDetails. The code should raise an exception if this field is missing rather than using defensive coding with .get(), as its absence indicates a data integrity issue that should be caught immediately with a fail-fast approach.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-07-15T02:02:56.052Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 929
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py:1809-1814
Timestamp: 2025-07-15T02:02:56.052Z
Learning: In the CompactConnect codebase, compact values come from custom attributes on the user's Cognito account, not from user input. When get_compact_configuration raises CCNotFoundException, it should be allowed to propagate as a 500 error (CCInternalException) to trigger alerts for system configuration issues, rather than being treated as a 4xx user error.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-04-29T02:10:38.400Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 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.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-07-21T20:39:47.625Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 907
File: backend/compact-connect/lambdas/python/cognito-backup/handlers/cognito_backup.py:148-149
Timestamp: 2025-07-21T20:39:47.625Z
Learning: In the CompactConnect Cognito backup system, UserCreateDate and UserLastModifiedDate fields from the AWS Cognito ListUsers API are accessed using direct dictionary access (user_data['UserCreateDate']) rather than .get() method. This is intentional per landonshumway-ia's decision to fail fast if AWS breaks their documented API contract that guarantees these fields are always present. The KeyError that would result from missing fields should trigger alerts to detect API contract violations rather than being silently handled.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-06-17T19:05:36.255Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect 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.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-09-03T22:16:52.533Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1040
File: backend/compact-connect/stacks/api_stack/v1_api/staff_users.py:459-459
Timestamp: 2025-09-03T22:16:52.533Z
Learning: In the CompactConnect codebase, IAM permissions for Lambda functions should only be granted for API calls that are actually made in the code, following the principle of least privilege. Do not suggest additional permissions without first verifying they are needed by the implementation.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2025-08-22T15:49:43.755Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1016
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:194-224
Timestamp: 2025-08-22T15:49:43.755Z
Learning: The API Gateway handler in this CompactConnect codebase returns JSON-formatted responses even for non-200 status codes, so response.json() calls don't need try/catch protection in API client code.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
📚 Learning: 2026-01-05T22:50:09.696Z
Learnt from: landonshumway-ia
Repo: csg-org/CompactConnect PR: 1243
File: backend/compact-connect/tests/smoke/smoke_common.py:503-521
Timestamp: 2026-01-05T22:50:09.696Z
Learning: Enforce the rule: there is only one privilege record per provider per jurisdiction, and do not paginate when querying privilege records by jurisdiction. This applies across the codebase wherever privilege records are queried or tested (implementation and tests). Note that privilege update records can have multiple rows and require pagination. Implement checks and queries to assume a unique constraint for (provider, jurisdiction) on privilege records, and ensure any list endpoints or test smoke checks reflect no pagination for jurisdiction-based privilege queries while preserving pagination for privilege updates.
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (4)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py (4)
1-9: LGTM!Imports are appropriate and align with the handler's functionality.
11-16: LGTM!The endpoint resource constant and decorator configuration are well-structured. Admin authorization is correctly enforced at the compact level.
66-77: LGTM!Event publishing and return logic are correctly implemented. The event payload includes all necessary fields for downstream notification processing.
50-51: The schema already validatesmilitaryStatusagainst the enum values viaOneOf([entry.value for entry in MilitaryAuditStatus]). Only valid enum value strings ('approved', 'declined') reach line 51, soMilitaryAuditStatus(military_status_str)is safe and cannot raiseValueError.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/military_audit.py
Show resolved
Hide resolved
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to go.
4 participants
Compact directors need the ability to view (via download) and approve or decline military documents. This also includes an automatic notification to the licensee when the document is approved or declined.
Requirements List
Description List
militaryAuditendpoint for compact admins to submit audit results.Testing List
run compact-connect/bin/download_oas30.pyCloses #1172
Summary by CodeRabbit
New Features
Enhancements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.