Feat/update practitioner email #877
Conversation
WalkthroughThis change implements the backend functionality for practitioners to update their account email addresses, including verification workflows. It introduces new API endpoints, database schema changes, email notification templates, service methods, and comprehensive automated and smoke tests to support and validate the email update and verification process. Changes
Sequence Diagram(s)sequenceDiagram
participant Practitioner
participant API Gateway
participant ProviderUsersLambda
participant Cognito
participant DynamoDB
participant EmailService
Practitioner->>API Gateway: PATCH /v1/provider-users/me/email {newEmailAddress}
API Gateway->>ProviderUsersLambda: Invoke
ProviderUsersLambda->>Cognito: Check if email exists
Cognito-->>ProviderUsersLambda: Exists/Not exists
alt Email available
ProviderUsersLambda->>DynamoDB: Store pendingEmailAddress, verificationCode, expiry
ProviderUsersLambda->>EmailService: Send verification code to new email
EmailService-->>ProviderUsersLambda: Success/Failure
ProviderUsersLambda-->>API Gateway: 200 OK
else Email taken
ProviderUsersLambda-->>API Gateway: 400 Error
end
Practitioner->>API Gateway: POST /v1/provider-users/me/email/verify {verificationCode}
API Gateway->>ProviderUsersLambda: Invoke
ProviderUsersLambda->>DynamoDB: Get pending verification data
alt Code valid and not expired
ProviderUsersLambda->>Cognito: Update email and verify
Cognito-->>ProviderUsersLambda: Success
ProviderUsersLambda->>DynamoDB: Finalize email update, clear pending fields
ProviderUsersLambda->>EmailService: Notify old email address
EmailService-->>ProviderUsersLambda: (optional) Success/Failure
ProviderUsersLambda-->>API Gateway: 200 OK
else Code invalid/expired
ProviderUsersLambda->>DynamoDB: Clear pending fields (if expired)
ProviderUsersLambda-->>API Gateway: 400 Error
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsbackend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (9)⏰ 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)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2567-2611: Add missing type hint and consider refactoring method signature.The method implementation is solid with proper error handling and conditional updates. However, there are two minor improvements needed:
- The
verification_expiryparameter is missing a type hint- The method has 6 parameters, which exceeds typical guidelines for method complexity
Apply this diff to add the missing type hint:
def update_provider_email_verification_data( self, *, compact: str, provider_id: str, pending_email_address: str, verification_code: str, - verification_expiry, + verification_expiry: datetime, ) -> None:Consider creating a data class for the verification parameters to reduce the argument count:
from dataclasses import dataclass from datetime import datetime @dataclass class EmailVerificationData: pending_email_address: str verification_code: str verification_expiry: datetime def update_provider_email_verification_data( self, *, compact: str, provider_id: str, verification_data: EmailVerificationData, ) -> None:backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (4)
23-36: Helper method is well-structured but could be more reusable.The helper method correctly sets up test data and event structure. However, it could benefit from parameter defaults to make it more flexible for different test scenarios.
Consider adding default parameters to make the helper more flexible:
-def _when_testing_provider_user_event_with_custom_claims(self, new_email=TEST_NEW_EMAIL): +def _when_testing_provider_user_event_with_custom_claims(self, new_email=TEST_NEW_EMAIL, old_email=TEST_OLD_EMAIL): self.test_data_generator.put_default_provider_record_in_provider_table( value_overrides={ - 'compactConnectRegisteredEmailAddress': TEST_OLD_EMAIL, + 'compactConnectRegisteredEmailAddress': old_email, } )
66-76: Test covers invalid email format but could be more comprehensive.The test correctly validates invalid email format rejection. However, it only tests one invalid format. Consider testing additional invalid email patterns for more robust validation.
Consider testing multiple invalid email formats:
@patch('cc_common.config._Config.email_service_client') def test_endpoint_returns_400_for_various_invalid_email_formats(self, mock_email_service_client): invalid_emails = ['invalid-email', 'test@', '@domain.com', '[email protected]'] for invalid_email in invalid_emails: with self.subTest(email=invalid_email): event = self._when_testing_provider_user_event_with_custom_claims(new_email=invalid_email) resp = provider_users_api_handler(event, self.mock_context) self.assertEqual(400, resp['statusCode'])
294-308: Expiration test uses hardcoded dates that could be fragile.While the test correctly validates expired codes, using hardcoded dates makes it brittle. Consider using relative time calculations.
Use relative time for more maintainable tests:
+from datetime import timedelta + # Use expired verification code (expiry in the past) -expired_time = datetime.fromisoformat('2024-11-08T23:00:00+00:00') +expired_time = MOCK_CURRENT_TIME - timedelta(minutes=1) # 1 minute in the past event = self._when_testing_provider_user_event_with_custom_claims(expiration=expired_time)
315-325: Cognito user setup is comprehensive but repetitive.The Cognito user creation with all attributes is correct and thorough. However, this setup is repeated across multiple tests and could be extracted to a helper method.
Extract Cognito user creation to a helper method:
def _create_cognito_user_for_test(self, email=TEST_OLD_EMAIL): """Create a Cognito user with test attributes for email verification tests""" self.config.cognito_client.admin_create_user( UserPoolId=self.config.provider_user_pool_id, Username=email, UserAttributes=[ {'Name': 'email', 'Value': email}, {'Name': 'email_verified', 'Value': 'true'}, {'Name': 'custom:compact', 'Value': DEFAULT_COMPACT}, {'Name': 'custom:providerId', 'Value': DEFAULT_PROVIDER_ID}, ], MessageAction='SUPPRESS', )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(3 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py(1 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_users.py(4 hunks)backend/compact-connect/tests/app/test_api/test_provider_users_api.py(6 hunks)backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/compact-connect/stacks/api_stack/v1_api/provider_users.py
[refactor] 102-102: Too many arguments (6/5)
(R0913)
[refactor] 102-102: Too many positional arguments (6/5)
(R0917)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
[refactor] 2568-2568: Too many arguments (6/5)
(R0913)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (34)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
2153-2177: Well-implemented email update request model.The implementation follows the established patterns in the codebase with proper lazy initialization, consistent email validation constraints (matching those used elsewhere like lines 314, 1385), and appropriate security measures by disabling additional properties.
2179-2199: Appropriate verification code validation for email verification.The 4-digit numeric pattern is suitable for email verification codes and provides a good balance between security and usability. The model properly restricts additional properties and follows the same lazy loading pattern as other models in the class.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
266-266: LGTM! Enum addition follows established conventions.The new
EMAIL_CHANGEenum member is appropriately named and valued, fitting well within the existingUpdateCategoryenum structure to support the email change tracking functionality.backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json (1)
1-15: Well-designed schema for verification code validation.The schema correctly validates a 4-digit numeric verification code with appropriate constraints. The pattern
^[0-9]{4}$ensures exactly four digits, andadditionalProperties: falseprovides strict validation.backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json (1)
1-17: Appropriate email validation schema with reasonable constraints.The schema properly validates email addresses with sensible length limits (5-100 characters) and uses standard JSON Schema email format validation. The strict property validation ensures clean input.
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts (1)
147-172: Email notification handlers follow established patterns correctly.Both new case statements implement proper validation for recipients and template variables, and follow the same structure as existing handlers. Using
specificEmails[0]is appropriate for these provider-specific notifications.backend/compact-connect/lambdas/python/purchases/purchase_client.py (2)
324-325: Appropriate linting fix with clear explanation.The explicit
return {}statement addresses the linter warning while maintaining code clarity. Thenoqacomment correctly explains that this branch raises an exception and the return is unreachable.
512-513: Consistent linting fix maintains code quality.This follows the same pattern as the previous fix, appropriately addressing the linter warning with a clear explanation that the return statement is unreachable due to the exception being raised.
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (2)
611-671: LGTM: Comprehensive test coverage for provider email verification code.The test suite properly covers success scenarios with correct email parameters, error handling for missing recipients, and validation of required template variables. The assertions correctly verify the verification code presence in the email body and proper subject line.
673-733: LGTM: Well-structured tests for provider email change notification.The test implementation follows the established patterns and includes comprehensive validation of email content, proper error handling for missing recipients and template variables, and correct assertion of the new email address in the notification body.
backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts (2)
632-743: Excellent test coverage for email verification code functionality.The tests comprehensively validate all critical aspects:
- Verification code inclusion in email body
- Security warnings for unauthorized requests
- 15-minute expiry notifications
- Proper email headers and subjects
This thorough testing ensures the security and user experience aspects are properly implemented.
745-856: Comprehensive test suite for email change notifications.The tests effectively verify:
- Notification of email address change with new address
- Login instructions for users
- Security warnings for unauthorized changes
- Proper email headers and formatting
The security-focused testing approach is particularly valuable for this sensitive operation.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py (1)
85-110: Well-implemented properties for email verification workflow.The three new properties follow the established class patterns and provide clean access to the email verification data. The documentation clearly explains when these fields are present and their purpose in the verification process.
The implementation correctly uses
.get()to handle optional fields, maintaining consistency with other optional properties in the class.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py (2)
70-73: Proper schema implementation for email verification fields.The field definitions are well-implemented with appropriate validations:
pendingEmailAddressuses proper Email field validationemailVerificationCodeenforces exactly 4 characters as expected for the verification workflowemailVerificationExpiryuses DateTime for proper temporal handlingAll fields are correctly marked as optional since they only exist during the verification process.
205-208: Consistent schema implementation in the update record schema.The email verification fields are properly mirrored in the
ProviderUpdatePreviousRecordSchemawith identical validation constraints, ensuring consistency for audit trail purposes.backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py (2)
244-270: Well-implemented email verification code method.The method properly follows the established pattern in the class:
- Constructs the payload with correct template name and recipient type
- Uses appropriate parameter types and documentation
- Follows the same error handling pattern via
_invoke_lambdaThe implementation correctly sends the verification code to the new email address as expected for the verification workflow.
272-298: Properly implemented email change notification method.The method correctly implements the notification to the old email address:
- Uses the appropriate template name for change notifications
- Sends to the old email address as expected for security notifications
- Includes the new email address in template variables for user awareness
This security-focused approach properly notifies users of account changes, which is a best practice for sensitive operations.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py (2)
207-282: Well-structured email verification initiationThe implementation correctly handles the email verification flow with proper validation, Cognito uniqueness check, and atomic updates. The error handling appropriately distinguishes between expected scenarios (UserNotFoundException) and unexpected errors.
372-380: Good resilience pattern for notification failuresExcellent decision to not fail the entire email update operation if the notification to the old email fails. This ensures the primary operation succeeds while still logging the notification failure for monitoring.
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts (1)
377-425: Well-implemented email notification methodsBoth email notification methods are properly structured with:
- Clear and informative email content
- Appropriate logging for monitoring
- Consistent error handling with descriptive error messages
- Proper use of the existing email template system
backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py (1)
144-194: Comprehensive test coverage for new email methodsThe tests properly verify that both new email service methods invoke the Lambda with the correct parameters, including template names, recipient types, and template variables. The test structure follows the established pattern in the test suite.
backend/compact-connect/stacks/api_stack/v1_api/provider_users.py (2)
126-129: Appropriate permissions for email update operationsThe Lambda handler is correctly granted:
- Permission to invoke the email notification service for sending verification codes and notifications
- Cognito permissions to check email uniqueness (AdminGetUser) and update user attributes (AdminUpdateUserAttributes)
These are the minimal required permissions for the email update functionality.
210-240: Well-configured API endpoints for email operationsBoth email endpoints are properly configured with:
- Appropriate HTTP methods (PATCH for initiation, POST for verification)
- Request validation using API models
- Proper authorization using the provider users authorizer
- Consistent response models
backend/compact-connect/tests/app/test_api/test_provider_users_api.py (1)
415-579: Thorough API endpoint testingThe tests comprehensively verify:
- Correct resource hierarchy and paths
- Proper HTTP method configuration
- Authorization setup
- Request and response model validation with snapshots
- Lambda integration configuration
The test structure follows the established pattern and provides good coverage for the new email update endpoints.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (2)
2613-2645: LGTM! Clean implementation of verification data cleanup.The method correctly implements the cleanup functionality with:
- Proper use of DynamoDB REMOVE operation to clear verification fields
- Appropriate conditional checks and error handling
- Good logging and exception translation to domain-specific exceptions
2647-2720: Excellent implementation of atomic email update with proper audit trail.This method demonstrates several best practices:
- Uses DynamoDB transactions to ensure atomicity between email update and verification data cleanup
- Creates comprehensive audit trail with
ProviderUpdateDatato track the email change- Captures the "previous" state before making changes for compliance
- Proper error handling with domain-specific exception translation
The transaction structure ensures data consistency and the audit trail provides proper tracking of email changes.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (8)
1-18: Test setup and constants look good.The file header, imports, and test constants are well-organized. The ruff noqa comment appropriately addresses the mock email client parameter requirement.
51-63: Test correctly validates email conflict scenario.The test properly simulates a Cognito user already existing with the target email and verifies the appropriate 400 response. Good coverage of this edge case.
138-158: Email whitespace handling test is excellent.This test properly validates that emails are trimmed while preserving case sensitivity. This is an important edge case that's often overlooked.
160-203: Test correctly validates overwriting pending verification.The test properly ensures that new email change requests overwrite existing pending requests. The verification that the email service is called twice is a good assertion.
234-253: Helper method has good parameter flexibility.Unlike the PATCH test helper, this one properly accepts parameters for code and expiration, making it more reusable across different test scenarios.
383-402: Cognito verification test is thorough and well-structured.The test properly verifies that Cognito users are updated with correct attributes and handles the unpredictable
subattribute gracefully. The exact attribute matching is excellent.
474-478: Audit trail verification is comprehensive.The test properly validates that update records are created with correct audit information, including both old and new email values. This ensures proper compliance tracking.
503-506: Email notification verification is complete.The test correctly verifies that change notifications are sent to the old email address with proper parameters. This is a critical security feature for email changes.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Outdated
Show resolved
Hide resolved
...ct/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json (2)
506-641: Consolidate repeatedadverseActionsdefinitions
The inlineadverseActionsschema underlicensesduplicates the same structure that appears elsewhere. Extract this into a shared definition (#/definitions/adverseActions/#/definitions/adverseAction) and replace the inline block with a$ref. That will reduce duplication and ensure future updates only need to be made in one place. Consider also adding"description"for clarity.--- a/backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json +++ b/backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json @@ licenses.items.properties - "adverseActions": { - "items": { /* full adverseAction properties */ }, - "type": "array" - }, + "adverseActions": { + "$ref": "#/definitions/adverseActions" + }, ... +# At the end of the schema, add reusable definitions: +"definitions": { + "adverseActions": { + "type": "array", + "items": { "$ref": "#/definitions/adverseAction" } + }, + "adverseAction": { + "type": "object", + "properties": { /* copy the properties block once here */ }, + "required": [ /* copy the required list once here */ ] + } +},
1300-1435: Reuse sharedadverseActionsdefinition in privileges
TheadverseActionsblock underprivilegesis identical to the one underlicenses. Replace it with a reference to the shared definition to keep both sections in sync.@@ privileges.items.properties - "adverseActions": { - "items": { /* full adverseAction properties */ }, - "type": "array" - }, + "adverseActions": { "$ref": "#/definitions/adverseActions" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(5 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json(0 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (8)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py (1)
73-73: LGTM - Well-designed optional field addition.The
liftingUserfield addition is properly configured as optional (required=False) while preventing null values (allow_none=False), which is appropriate for tracking who lifted an adverse action when that information is available.backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json (4)
82-149: LGTM - Proper addition of license jurisdiction and type fields.The new
licenseJurisdictionandlicenseTypefields are well-defined with appropriate enum constraints matching the existing jurisdiction patterns in the schema.
187-499: LGTM - Comprehensive privilege history tracking schema.The history array schema properly captures privilege update events with detailed before/after state tracking. The nested structure for
previousandupdatedValuesprovides good audit trail capabilities.
500-628: LGTM - Well-structured adverse actions schema.The adverse actions schema includes all necessary fields for tracking adverse action events with proper date validation patterns and required field specifications.
635-643: LGTM - Updated required fields reflect schema enhancements.The addition of
licenseJurisdiction,licenseType,administratorSetStatus, andprivilegeIdto the required fields list properly enforces the new mandatory data elements.backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json (3)
506-641: LGTM - Consistent adverse actions schema for licenses.The adverse actions schema for licenses matches the pattern established in other schemas with appropriate field validation and required field specifications.
1300-1435: LGTM - Comprehensive adverse actions schema for privileges.The adverse actions schema for privileges includes additional context-specific fields like
clinicalPrivilegeActionCategoryandliftingUser, providing detailed tracking capabilities for privilege-related adverse actions.
624-637: Verify consistency with adverse action API schema.The required fields list includes
clinicalPrivilegeActionCategorybut notliftingUser, which aligns with the API schema whereliftingUseris optional. However, ensure this matches the actual implementation requirements.#!/bin/bash # Verify that the liftingUser field configuration is consistent across schemas echo "Checking liftingUser field configuration in adverse action schemas..." # Search for liftingUser in all schema files rg -A 3 -B 3 "liftingUser" --type json # Search for liftingUser in Python schema files rg -A 3 -B 3 "liftingUser" --type py echo "Checking required fields in adverse action schemas..." # Look for required arrays that include or exclude liftingUser rg -A 10 -B 2 '"required".*adverse|adverse.*"required"' --type json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (3)
74-75: Extract repeated verification code validation into a helper function.The verification code validation logic is duplicated. This violates the DRY principle and makes maintenance harder.
Replace both instances with a call to the helper function suggested above:
- if not verification_code or len(verification_code) != 4 or not verification_code.isdigit(): - raise SmokeTestFailureException('Verification code must be exactly 4 digits') + _validate_verification_code(verification_code) - if not restore_verification_code or len(restore_verification_code) != 4 or not restore_verification_code.isdigit(): - raise SmokeTestFailureException('Restore verification code must be exactly 4 digits') + _validate_verification_code(restore_verification_code, "Restore verification")Also applies to: 149-150
125-127: Improve exception handling specificity.Using bare
Exceptioncatches can mask specific errors and make debugging harder. Consider catching more specific exceptions where possible.- except Exception as e: # noqa: BLE001 + except (requests.RequestException, SmokeTestFailureException) as e: logger.error(f'Failed to authenticate with new email: {str(e)}') raise SmokeTestFailureException(f'Authentication with new email failed: {str(e)}') from e - except Exception as e: # noqa: BLE001 + except (requests.RequestException, SmokeTestFailureException) as e: logger.error(f'Failed to authenticate with restored email: {str(e)}') raise SmokeTestFailureException(f'Authentication with restored email failed: {str(e)}') from eAlso applies to: 182-184
51-56: Consider extracting API endpoint constants.The API endpoints are hardcoded throughout the file. Consider defining them as constants at the module level for better maintainability.
+# API endpoints +EMAIL_UPDATE_ENDPOINT = '/v1/provider-users/me/email' +EMAIL_VERIFY_ENDPOINT = '/v1/provider-users/me/email/verify' +PROVIDER_ME_ENDPOINT = '/v1/provider-users/me' # Then use in the requests: - url=f'{config.api_base_url}/v1/provider-users/me/email', + url=f'{config.api_base_url}{EMAIL_UPDATE_ENDPOINT}',Also applies to: 80-85, 133-138, 155-160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py(1 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[error] 2-2: block comment should start with '# '
(E265)
🪛 Pylint (3.3.7)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[refactor] 20-20: Too many local variables (21/15)
(R0914)
[refactor] 20-20: Too many branches (15/12)
(R0912)
[refactor] 20-20: Too many statements (79/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (3)
1-2: Shebang formatting is correct for executable Python scripts.The Flake8 E265 error about block comment spacing is a false positive here - shebangs don't require a space after the
#character.
192-252: Well-structured error scenario testing.The
test_email_update_error_scenarios()function is well-designed with focused, specific test cases for different error conditions. The function size and complexity are appropriate for its purpose.
255-261: Good separation of test execution.The main execution block properly loads the environment and runs error scenarios before the main test, which is a good practice for smoke testing - fail fast on basic validation errors.
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (1)
20-190: Address the complexity issues previously identified.The main test function remains overly complex with significant maintainability concerns. This issue was previously identified and specific refactoring suggestions were provided.
Please implement the helper functions suggested in the previous review to break down this monolithic test into smaller, focused functions for better maintainability and debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
- backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
🧰 Additional context used
🪛 GitHub Check: CheckLambdas
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
[failure] 725-725:
Expected blank line before this statement
🪛 GitHub Actions: Check-Lambda-JS
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
[error] 725-725: ESLint: Expected blank line before this statement (padding-line-between-statements)
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[error] 2-2: block comment should start with '# '
(E265)
🪛 Pylint (3.3.7)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[refactor] 20-20: Too many local variables (21/15)
(R0914)
[refactor] 20-20: Too many branches (15/12)
(R0912)
[refactor] 20-20: Too many statements (79/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (4)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts (1)
611-682: LGTM! Comprehensive test coverage for email verification code functionality.The test suite thoroughly covers the
providerEmailVerificationCodetemplate with proper assertions for:
- Successful email sending with correct content validation
- Error handling for missing recipients and template variables
- Detailed HTML content verification including verification code display
The implementation follows the established testing patterns in the file.
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (3)
192-253: LGTM! Well-structured error scenario testing.The error scenario tests are well-organized and cover important edge cases:
- Invalid email format validation
- Invalid verification code format validation
- No pending verification state validation
This provides good coverage of the API's error handling capabilities.
255-261: Comprehensive smoke test implementation.The overall smoke test provides excellent end-to-end coverage of the practitioner email update functionality, including both happy path and error scenarios. The manual verification approach is appropriate for smoke testing actual email delivery.
2-2: Fix block comment formatting.-#!/usr/bin/env python3 +# !/usr/bin/env python3Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (1)
22-192: Consider the previous refactoring suggestion for function complexity.The main test function remains overly complex with 79 statements, 21 local variables, and 15 branches, as previously identified. This impacts maintainability and debugging capabilities.
🧹 Nitpick comments (4)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (4)
2-2: Fix the block comment formatting.The shebang comment doesn't follow proper formatting standards.
-#!/usr/bin/env python3 +# !/usr/bin/env python3
44-47: Enhance input validation for the new email address.The current validation only checks if the email is provided and different from the current email, but doesn't validate the email format.
+import re + +def _validate_email_format(email: str) -> bool: + """Validate email format using a simple regex.""" + pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$' + return re.match(pattern, email) is not None new_email = input(f'Enter a valid email address to update the practitioner to (current: {original_email}): ') -if not new_email or new_email == original_email: - raise SmokeTestFailureException('New email address must be provided and different from current email') +if not new_email or new_email == original_email: + raise SmokeTestFailureException('New email address must be provided and different from current email') + +if not _validate_email_format(new_email): + raise SmokeTestFailureException('Invalid email address format')
127-129: Improve exception handling specificity.Using broad exception handling with
Exceptionmakes debugging difficult and may mask specific issues.- except Exception as e: # noqa: BLE001 + except (requests.RequestException, KeyError, TypeError) as e: logger.error(f'Failed to authenticate with new email: {str(e)}') raise SmokeTestFailureException(f'Authentication with new email failed: {str(e)}') from e- except Exception as e: # noqa: BLE001 + except (requests.RequestException, KeyError, TypeError) as e: logger.error(f'Failed to authenticate with restored email: {str(e)}') raise SmokeTestFailureException(f'Authentication with restored email failed: {str(e)}') from eAlso applies to: 184-186
262-263: Consider adding setup/teardown or test isolation.Running both tests sequentially without isolation could lead to state conflicts, especially if the first test fails partway through and leaves the system in an inconsistent state.
if __name__ == '__main__': # Load environment variables from smoke_tests_env.json load_smoke_test_env() - # Run tests - test_email_update_error_scenarios() - test_practitioner_email_update() + # Run tests with proper error handling + try: + logger.info('Running email update error scenarios test') + test_email_update_error_scenarios() + logger.info('Error scenarios test completed successfully') + + logger.info('Running main email update smoke test') + test_practitioner_email_update() + logger.info('All tests completed successfully') + except SmokeTestFailureException as e: + logger.error(f'Smoke test failed: {e}') + exit(1) + except Exception as e: + logger.error(f'Unexpected error during smoke testing: {e}') + exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[error] 2-2: block comment should start with '# '
(E265)
🪛 Pylint (3.3.7)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py
[refactor] 22-22: Too many local variables (21/15)
(R0914)
[refactor] 22-22: Too many branches (15/12)
(R0912)
[refactor] 22-22: Too many statements (79/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (2)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_test.py (2)
194-255: Excellent error scenario coverage.The error scenarios test function is well-structured and covers important edge cases including invalid email format, invalid verification code format, and attempting verification without pending updates.
106-106: Verify that the test password is not hardcoded for security.The test uses
config.test_provider_user_passwordwhich should be sourced from secure configuration, not hardcoded in the codebase.#!/bin/bash # Description: Check if test password is hardcoded anywhere in the codebase # Expected: Password should come from environment variables or secure config # Search for hardcoded password patterns rg -i "test_provider_user_password.*=" --type py -A 2 -B 2 # Search for any password-like hardcoded values rg -i "(password|pwd).*=.*['\"]" --type py -A 1 -B 1
|
Screenshots of the generated emails: Verification code email: Email Change Confirmation email: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (2)
2-2: Fix comment formatting.The shebang line should start with
#!without a space after the#.-#!/usr/bin/env python3 +#!/usr/bin/env python3
46-47: Improve input validation and reduce code duplication.The email and verification code validation logic is duplicated and could be more robust.
Consider extracting validation functions:
+def _validate_email_input(new_email: str, current_email: str) -> str: + """Validate new email input from user.""" + if not new_email or not new_email.strip(): + raise SmokeTestFailureException('New email address must be provided') + + new_email = new_email.strip() + if new_email == current_email: + raise SmokeTestFailureException('New email address must be different from current email') + + # Basic email format validation + if '@' not in new_email or '.' not in new_email.split('@')[-1]: + raise SmokeTestFailureException('Please provide a valid email address format') + + return new_email +def _validate_verification_code(code: str, purpose: str = "verification") -> str: + """Validate verification code format.""" + code = code.strip() + if not code or len(code) != 4 or not code.isdigit(): + raise SmokeTestFailureException(f'{purpose.capitalize()} code must be exactly 4 digits') + return codeAlso applies to: 76-77, 151-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/compact-connect/lambdas/python/common/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/common/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/data-events/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/purchases/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/purchases/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt(3 hunks)backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt(3 hunks)backend/compact-connect/requirements-dev.txt(5 hunks)backend/compact-connect/requirements.txt(1 hunks)backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py(1 hunks)backend/multi-account/control-tower/requirements-dev.txt(1 hunks)backend/multi-account/control-tower/requirements.txt(1 hunks)backend/multi-account/log-aggregation/requirements-dev.txt(1 hunks)backend/multi-account/log-aggregation/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt
- backend/compact-connect/lambdas/python/purchases/requirements.txt
- backend/multi-account/control-tower/requirements.txt
- backend/compact-connect/lambdas/python/common/requirements-dev.txt
- backend/multi-account/log-aggregation/requirements.txt
- backend/compact-connect/lambdas/python/data-events/requirements-dev.txt
- backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt
- backend/compact-connect/lambdas/python/common/requirements.txt
- backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt
- backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt
- backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt
- backend/compact-connect/requirements.txt
- backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt
- backend/compact-connect/lambdas/python/purchases/requirements-dev.txt
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
[error] 2-2: block comment should start with '# '
(E265)
🪛 Pylint (3.3.7)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
[refactor] 22-22: Too many local variables (21/15)
(R0914)
[refactor] 22-22: Too many branches (15/12)
(R0912)
[refactor] 22-22: Too many statements (79/50)
(R0915)
🪛 LanguageTool
backend/compact-connect/requirements-dev.txt
[duplication] ~82-~82: Possible typo: you repeated a word.
Context: ...act-connect/requirements-dev.in # pytest-cov pytest-cov==6.2.1 # via -r compact-connect/req...
(ENGLISH_WORD_REPEAT_RULE)
backend/multi-account/control-tower/requirements-dev.txt
[duplication] ~14-~14: Possible typo: you repeated a word.
Context: ...# via pytest pygments==2.19.2 # via pytest pytest==8.4.1 # via -r multi-account/contr...
(ENGLISH_WORD_REPEAT_RULE)
backend/multi-account/log-aggregation/requirements-dev.txt
[duplication] ~14-~14: Possible typo: you repeated a word.
Context: ...# via pytest pygments==2.19.2 # via pytest pytest==8.4.1 # via -r multi-account/log-a...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (16)
backend/multi-account/log-aggregation/requirements-dev.txt (2)
13-13: Bump pygments patch version
Updated pygments from 2.19.1 to 2.19.2 to stay in sync with other backend modules.
15-15: Align pytest to latest patch release
Upgraded pytest from 8.4.0 to 8.4.1 for consistent dev dependency versions across the codebase.backend/multi-account/control-tower/requirements-dev.txt (2)
13-13: Verify bump ofpygmentsin source.infile.This file is auto-generated by
pip-compile. Please confirm that you updatedmulti-account/control-tower/requirements-dev.into bumppygmentsto2.19.2and then ranpip-compileto regenerate this file.
15-15: Verify bump ofpytestin source.infile.Similarly, ensure that
pytestwas bumped to8.4.1inmulti-account/control-tower/requirements-dev.inand that you regenerated the lock file viapip-compile.backend/compact-connect/requirements-dev.txt (10)
15-15: Certifi bumped to 2025.6.15
Upgrade is consistent with other services and pulls in the latest CA bundle fixes.
21-21: coverage[toml] pinned to 7.9.1
Version bump aligns with pytest-cov updates and brings the latest TOML support.
43-43: msgpack updated to 1.1.1
Minor patch release—no breaking changes expected.
64-66: Expanded pluggy comment annotations
Adding bothpytestandpytest-covas dependents improves clarity on why pluggy is pulled in.
67-67: No change inpy-serializableversion—annotation appears as part of context rather than an update.
69-69: pygments bumped to 2.19.2
Patch bump—captures recent fixes and is backward-compatible.
79-79: pytest upgraded to 8.4.1
Brings in latest bug fixes; matches other backend modules.
83-83: pytest-cov updated to 6.2.1
Ensures coverage plugin compatibility with the new pytest version.
93-93: ruff pinned at 0.12.0
Linting tool version bump—no adverse effects expected.
101-101: urllib3 upgraded to 2.5.0
Pulls in the latest network-layer improvements and security patches.backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (2)
194-255: Well-structured error scenarios testing.This function demonstrates good testing practices with focused, specific error condition tests. The structure and length are appropriate for this type of testing.
257-263: Good test execution order.Running error scenarios before the main test is a logical approach that can catch API issues early without requiring manual email verification input.
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/purchase_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/purchase_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.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/common/cc_common/data_model/data_client.py (1)
2568-2576: Add type hint for datetime parameter to maintain consistency.Based on past feedback encouraging consistency in handling date/datetime objects, consider adding a type hint for the verification_expiry parameter.
def update_provider_email_verification_data( self, *, compact: str, provider_id: str, pending_email_address: str, verification_code: str, - verification_expiry: datetime, + verification_expiry: datetime, ) -> None:Consider refactoring to reduce argument count.
The method has 6 arguments which exceeds the typical limit. Consider grouping related parameters into a data class or dictionary.
from dataclasses import dataclass @dataclass class EmailVerificationData: pending_email_address: str verification_code: str verification_expiry: datetime def update_provider_email_verification_data( self, *, compact: str, provider_id: str, verification_data: EmailVerificationData, ) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
undefined
<retrieved_learning>
Learnt from: landonshumway-ia
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.
</retrieved_learning>
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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 CCDataClass in CompactConnect supports three initialization methods: 1) load_from_database_record() to load from a database record, 2) pass in a dict to constructor (without db keys like pk/sk), and 3) initialize empty and use setters to populate values.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (4)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.842Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
[refactor] 2568-2568: Too many arguments (6/5)
(R0913)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
[refactor] 15-15: Too few public methods (1/2)
(R0903)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
[refactor] 151-151: Too few public methods (1/2)
(R0903)
[refactor] 164-164: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (11)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (3)
7-7: LGTM - Import addition is appropriate.The import of
CCRequestSchemais correctly added to support the new schema classes that provide input whitespace trimming functionality.
151-162: LGTM - Email update schema follows established patterns.The
ProviderEmailUpdateRequestSchemacorrectly:
- Inherits from
CCRequestSchemafor input trimming- Uses
- Follows the codebase pattern of
required=True, allow_none=Falsefor required fields- Includes appropriate documentation
164-175: LGTM - Verification schema implementation is solid.The
ProviderEmailVerificationRequestSchemacorrectly:
- Validates verification code length to exactly 4 characters using
Length(min=4, max=4)- Uses appropriate field configuration following established patterns
- Provides clear documentation for API usage
The 4-character length validation aligns with the 4-digit verification code mentioned in the PR objectives.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
2-2: LGTM - Import addition supports new functionality.The datetime import is properly added alongside the existing date import to support the new email verification methods.
2588-2611: LGTM - Robust error handling and conditional updates.The implementation correctly uses conditional expressions to ensure the provider exists before updating, with appropriate exception handling for different failure scenarios.
2628-2645: LGTM - Clean removal of verification data.The REMOVE operation properly clears all verification-related fields while maintaining the dateOfUpdate field.
2682-2720: Excellent atomic transaction implementation! 👍The transaction properly ensures data consistency by atomically updating the provider's email address and creating an audit record. This follows the established pattern of self-consistent updates that previous reviewers have appreciated.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (4)
9-9: LGTM: Import addition supports new functionality.The
pre_loadimport is correctly added to support the newCCRequestSchemaclass implementation.
15-54: Well-implemented base schema with comprehensive input sanitization.The
CCRequestSchemaclass provides excellent common functionality for request processing:
- Proper recursive handling of nested data structures (dicts, lists)
- Clear documentation explaining the purpose and usage
- Correct use of marshmallow's
@pre_loaddecorator- Defensive programming with type checking
The static analysis tool's "too few public methods" warning is a false positive here - this is a marshmallow Schema subclass that inherits many methods, and having a single focused public method for a utility base class is appropriate.
307-307: LGTM: Email change enum value follows existing pattern.The new
EMAIL_CHANGE = 'emailChange'enum value is correctly implemented and consistent with the existing camelCase naming pattern used in this enum.
355-358: LGTM: New clinical privilege categories align with NPDB standards.The four new clinical privilege action categories are properly implemented:
- Follow the existing naming and formatting patterns
- Align with NPDB standards as referenced in the class docstring
- Maintain consistency with the existing enum structure
landonshumway-ia
force-pushed
the
feat/update-practitioner-email
branch
from
58a5925 to
7d6a55b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
607-609: Fix error handling inconsistency.There's an inconsistency in exception handling between this method and
clear_provider_email_verification_data. When the provider is not found:
- This method raises
CCInternalException(line 609)- The other method raises
CCNotFoundException(line 643)For the same error condition (provider not found), these should raise the same exception type for consistency.
if e.response['Error']['Code'] == 'ConditionalCheckFailedException': logger.error('Provider not found', error=str(e)) - raise CCInternalException('Provider not found') from e + raise CCNotFoundException('Provider not found') from e
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (1)
90-117: Comprehensive database state validation with proper verification code checking.The test thoroughly validates that pending email fields are set correctly, including the fix from the previous review comment that ensures the verification code is numeric.
🧹 Nitpick comments (2)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (1)
1-2: Fix comment formatting.The shebang comment doesn't follow the expected format.
-#!/usr/bin/env python3 +# !/usr/bin/env python3backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2567-2576: Add type hint for verification_expiry parameter.Based on past review feedback requesting consistency in type hints for date/datetime parameters, please add a type hint to the
verification_expiryparameter.def update_provider_email_verification_data( self, *, compact: str, provider_id: str, pending_email_address: str, verification_code: str, - verification_expiry: datetime, + verification_expiry: datetime, ) -> None:Note: The static analysis tool flags this method for having too many arguments (6/5). Consider if any parameters could be combined into a data structure if this becomes a maintenance concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(5 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_users.py(4 hunks)backend/compact-connect/tests/app/test_api/test_provider_users_api.py(6 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json(0 hunks)backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/init.py
🚧 Files skipped from review as they are similar to previous changes (14)
- backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
- backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
- backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
- backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
- backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py
- backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
- backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
- backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
- backend/compact-connect/tests/app/test_api/test_provider_users_api.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
- backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (3)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (3)
undefined
<retrieved_learning>
Learnt from: landonshumway-ia
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: jusdino
PR: #864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The licenseJurisdiction field is a required field in the provider data API response from the /v1/providers/users/me endpoint, so it can be accessed directly without defensive programming checks.
</retrieved_learning>
<retrieved_learning>
Learnt from: landonshumway-ia
PR: #769
File: backend/compact-connect/lambdas/python/common/tests/resources/dynamo/provider.json:5-5
Timestamp: 2025-04-29T01:57:43.684Z
Learning: The provider.json test data contains both "providerDateOfUpdate" and "dateOfUpdate" fields which serve different purposes, and both should be maintained in the test files.
</retrieved_learning>
backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json (5)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: jusdino
PR: csg-org/CompactConnect#864
File: backend/compact-connect/tests/smoke/encumbrance_smoke_tests.py:256-262
Timestamp: 2025-06-18T21:57:02.978Z
Learning: The `licenseJurisdiction` field is a required field in the provider data API response from the `/v1/providers/users/me` endpoint, so it can be accessed directly without defensive programming checks.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:397-401
Timestamp: 2025-04-29T01:59:51.222Z
Learning: In the CompactConnect project, validation constraints like enum values should be defined only in runtime code (Lambda) rather than duplicating them in CDK API schema definitions. This applies to fields like clinicalPrivilegeActionCategory and other similar enumerations.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py:138-147
Timestamp: 2025-04-29T02:52:40.532Z
Learning: In CompactConnect tests, hardcoded values (like license type abbreviations 'slp') in test queries are sometimes used intentionally rather than using dynamic lookups. This is a deliberate design decision to make tests fail if default test data changes, requiring developers to consciously update related tests.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
undefined
<retrieved_learning>
Learnt from: landonshumway-ia
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.
</retrieved_learning>
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (2)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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 CCDataClass in CompactConnect supports three initialization methods: 1) load_from_database_record() to load from a database record, 2) pass in a dict to constructor (without db keys like pk/sk), and 3) initialize empty and use setters to populate values.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (4)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/record.py:40-43
Timestamp: 2025-04-29T02:18:11.099Z
Learning: In the CompactConnect codebase, API design follows a pattern where fields are either provided with valid values or omitted entirely. Null values are not allowed, even for optional fields. This applies to Marshmallow schemas where fields are configured with `required=False, allow_none=False`.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#769
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py:87-90
Timestamp: 2025-04-29T21:09:04.842Z
Learning: In the CCDataClass implementation, validation is performed during both the dump() and load() processes in Marshmallow, so calling dump() first still results in ValidationError if the provided data is invalid.
backend/compact-connect/stacks/api_stack/v1_api/provider_users.py (3)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (5)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/tests/app/test_event_listener.py:29-222
Timestamp: 2025-06-04T20:07:09.680Z
Learning: In the CompactConnect project, test methods that include property snapshots for different AWS resource configurations (like event listeners) should be kept decoupled rather than extracted into common helper methods, even if they appear similar. This avoids complex parameter passing and maintains test independence when snapshots may vary between configurations.
🧬 Code Graph Analysis (2)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (4)
backend/compact-connect/tests/app/test_event_listener.py (1)
get_context(19-27)backend/compact-connect/tests/app/test_transaction_monitoring.py (1)
get_context(27-34)backend/compact-connect/tests/app/test_pipeline.py (2)
get_context(22-31)get_context(298-308)backend/compact-connect/common_constructs/stack.py (1)
license_type_names(62-64)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
CCRequestSchema(15-53)
🪛 Pylint (3.3.7)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
[refactor] 2568-2568: Too many arguments (6/5)
(R0913)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
[refactor] 15-15: Too few public methods (1/2)
(R0903)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
[refactor] 151-151: Too few public methods (1/2)
(R0903)
[refactor] 164-164: Too few public methods (1/2)
(R0903)
backend/compact-connect/stacks/api_stack/v1_api/provider_users.py
[refactor] 102-102: Too many arguments (6/5)
(R0913)
[refactor] 102-102: Too many positional arguments (6/5)
(R0917)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
[refactor] 22-22: Too many local variables (21/15)
(R0914)
[refactor] 22-22: Too many branches (15/12)
(R0912)
[refactor] 22-22: Too many statements (79/50)
(R0915)
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
[error] 2-2: block comment should start with '# '
(E265)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (38)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py (1)
151-174: Clean implementation of request validation schemas.Both schemas follow the project's validation patterns correctly:
- Proper inheritance from
CCRequestSchemafor input sanitization- Email validation with
- Verification code validation with exact length constraint
- Consistent use of
required=True, allow_none=Falsebackend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (2)
22-192: Comprehensive smoke test implementation.The test provides thorough coverage of the email update workflow:
- Complete round-trip testing (update and restore)
- Manual verification code input for realistic testing
- Proper authentication testing with new credentials
- Good error handling and validation throughout
Based on project conventions, the single large function structure is appropriate for smoke tests to maintain narrative flow.
194-255: Good coverage of error scenarios.The error testing covers important validation cases:
- Invalid email format validation
- Invalid verification code format
- Missing pending verification state
backend/compact-connect/stacks/api_stack/v1_api/provider_users.py (3)
94-100: Well-structured API resource hierarchy.The email update endpoints are properly organized:
/me/emailfor PATCH operations/me/email/verifyfor POST verification- Clean resource nesting following RESTful conventions
126-129: Appropriate permissions for email update operations.The Lambda handler is granted the necessary permissions for:
- Email notification service invocation
- Cognito user attribute management (
AdminGetUser,AdminUpdateUserAttributes)These permissions are correctly scoped for the email update functionality.
210-241: Consistent API method configuration.Both email endpoints follow the established patterns:
- Proper request validation and models
- Correct authorizer usage
- Consistent timeout and integration settings
- Appropriate response models
backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json (1)
82-149: Comprehensive schema enhancements for privilege tracking.The schema additions provide detailed support for:
- License jurisdiction and type tracking in privileges
- Complete privilege update history with previous/updated values
- Adverse action tracking with comprehensive metadata
- Proper enum constraints for all jurisdiction and license type fields
The schema structure is well-organized and maintains consistency with existing patterns.
Also applies to: 187-499, 500-628, 635-642
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (3)
15-54: Excellent base schema implementation for input sanitization.The
CCRequestSchemaprovides robust input processing:
- Recursive whitespace stripping for nested data structures
- Safe type checking to avoid errors
- Clear documentation of purpose and usage
- Proper use of
@pre_loadhook for preprocessingThis will ensure consistent input sanitization across all request schemas.
307-307: Appropriate enum addition for email tracking.The
EMAIL_CHANGEenum member correctly supports tracking email update operations in provider update records.
355-358: Updated clinical privilege action categories.The enum updates expand the available adverse action categories with relevant options for compliance and misconduct tracking.
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (5)
1157-1207: Well-structured adverse actions schema for licenses.The adverse actions schema is comprehensive and follows consistent patterns with other schemas in the file. The required and optional fields provide appropriate coverage for adverse action tracking.
1293-1343: Good consistency with license adverse actions schema.The privilege adverse actions schema maintains the same structure as the license version, ensuring consistency across the API for adverse action tracking.
2102-2109: Appropriate enhancement of public privilege response.Adding these required fields (licenseJurisdiction, licenseType, administratorSetStatus, privilegeId) provides valuable context in the public API response while maintaining appropriate information boundaries.
2389-2435: Well-designed email request models with proper validation.Both email request models are appropriately structured:
patch_provider_email_request_model: Uses proper email format validation with reasonable length constraintspost_provider_email_verify_request_model: Uses secure 4-digit numeric pattern validationThe models follow established patterns in the codebase and support the secure email verification workflow.
2132-2266: Verify clinicalPrivilegeActionCategory in public adverseActions schemaThe public
adverseActionsdefinition in
backend/compact-connect/stacks/api_stack/v1_api/api_model.py(approx. lines 2228–2240) omits theclinicalPrivilegeActionCategoryfield, whereas the private API schema requires it. Please confirm whether this omission is intentional:
- If it should be exposed publicly, add
'clinicalPrivilegeActionCategory'to therequiredarray and include itsJsonSchemaentry underproperties.- If it’s deliberately excluded from the public API, document this decision in the schema comments or API spec.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (4)
2-2: LGTM: Import addition is appropriate.The
datetimeimport is necessary for the new email verification methods that handle timestamp data.
2613-2646: LGTM: Clean implementation of verification data cleanup.The method correctly uses the REMOVE operation to clear verification fields and handles the provider-not-found case appropriately with
CCNotFoundException.
2684-2685: Excellent use of atomic transactions!This transaction ensures both the email update and audit record creation happen atomically, maintaining data consistency. This follows the pattern appreciated in previous reviews.
2647-2721: LGTM: Well-designed email update completion with proper audit trail.The method demonstrates several best practices:
- Retrieves current state before modification for audit purposes
- Uses atomic transaction to ensure consistency between email update and audit record creation
- Comprehensive error handling with appropriate exception types
- Proper use of REMOVE operation to clear verification data
The implementation ensures data integrity while maintaining a complete audit trail of the email change.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (19)
1-18: Well-structured test setup with clear constants.The imports, constants, and mock time configuration provide a solid foundation for deterministic testing of time-sensitive email verification functionality.
20-48: Excellent helper method design for test reusability.The helper methods provide clean abstractions for test setup while maintaining readability and following established testing patterns in the project.
50-64: Comprehensive test for email conflict validation.The test properly simulates an existing Cognito user and verifies the correct error response when attempting to use an already-taken email address.
65-76: Effective validation of email format checking.The test correctly verifies that invalid email formats are rejected with appropriate error messages.
118-138: Excellent integration test with controlled randomness.The test effectively validates email service integration by mocking random number generation and verifying the exact parameters passed to the email service.
140-162: Good input sanitization test with case preservation.The test effectively validates that whitespace is trimmed while email case is preserved, covering important input handling edge cases.
163-207: Comprehensive test for pending verification overwrite behavior.The test effectively validates that subsequent email change requests properly overwrite previous pending verifications, which is crucial for user experience.
208-232: Precise validation of expiry time calculation.The test accurately verifies that verification codes expire at the correct time, ensuring proper security behavior.
234-256: Consistent test class structure with proper setup helpers.The verification test class follows the same well-established patterns and provides clean helper methods for test scenario setup.
258-282: Appropriate validation for missing pending email state.The test correctly ensures that verification cannot proceed without a pending email change, with a clear and actionable error message.
284-296: Proper security validation for incorrect verification codes.The test ensures that invalid verification codes are properly rejected with appropriate error responses.
297-311: Essential security test for expired verification codes.The test properly validates that expired verification codes are rejected with helpful guidance for the user to retry.
313-336: Clean validation of successful verification flow.The test properly validates the happy path with appropriate setup and success response verification.
338-361: Important data cleanup validation for expired codes.The test ensures proper cleanup of pending verification data when codes expire, maintaining good data hygiene.
363-406: Comprehensive Cognito user attribute validation.The test thoroughly verifies that Cognito user attributes are updated correctly, with appropriate handling of auto-generated fields like 'sub'.
407-444: Thorough validation of DynamoDB state changes.The test ensures that provider records are properly updated and pending fields are cleared after successful email verification.
445-482: Essential audit trail validation for compliance.The test ensures proper creation of audit records with correct update type and historical data preservation for email changes.
483-510: Important security notification validation.The test ensures that users are properly notified at their old email address when their email changes, which is crucial for security.
511-565: Excellent edge case coverage for race condition scenarios.The test effectively handles the realistic scenario where an email becomes unavailable during verification, ensuring proper error handling and state cleanup.
|
@jlkravitz This is ready for your review. Thanks |
- use schema to strip whitespace - remove optional pending email fields from update record - add type hint for datetime
landonshumway-ia
force-pushed
the
feat/update-practitioner-email
branch
from
ff60361 to
3a1cf9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (1)
2-2: Fix comment formatting.The shebang comment is missing a space after
#.-#!/usr/bin/env python3 +# !/usr/bin/env python3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts(1 hunks)backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(5 hunks)backend/compact-connect/stacks/api_stack/v1_api/provider_users.py(4 hunks)backend/compact-connect/tests/app/test_api/test_provider_users_api.py(6 hunks)backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json(1 hunks)backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json(0 hunks)backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/tests/resources/snapshots/QUERY_PROVIDERS_RESPONSE_SCHEMA.json
🚧 Files skipped from review as they are similar to previous changes (21)
- backend/compact-connect/tests/resources/snapshots/POST_PROVIDER_USERS_EMAIL_VERIFY_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/adverse_action/api.py
- backend/compact-connect/tests/resources/snapshots/PATCH_PROVIDER_USERS_EMAIL_REQUEST_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/lambdas/nodejs/email-notification-service/lambda.ts
- backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/init.py
- backend/compact-connect/lambdas/nodejs/tests/email-notification-service.test.ts
- backend/compact-connect/stacks/api_stack/v1_api/provider_users.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
- backend/compact-connect/lambdas/python/common/cc_common/email_service_client.py
- backend/compact-connect/lambdas/nodejs/lib/email/email-notification-service.ts
- backend/compact-connect/tests/resources/snapshots/PUBLIC_GET_PROVIDER_RESPONSE_SCHEMA.json
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/tests/resources/snapshots/PROVIDER_USER_RESPONSE_SCHEMA.json
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_email_service_client.py
- backend/compact-connect/tests/app/test_api/test_provider_users_api.py
- backend/compact-connect/tests/resources/snapshots/GET_PROVIDER_RESPONSE_SCHEMA.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (3)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/migration/provider_user_pool_migration_551/main.py:35-39
Timestamp: 2025-06-10T03:16:16.896Z
Learning: In the provider user pool migration (provider_user_pool_migration_551), the FilterExpression intentionally only checks for the existence of compactConnectRegisteredEmailAddress. The migration should only remove currentHomeJurisdiction if compactConnectRegisteredEmailAddress is also present, targeting records that went through the old registration process. Records with only currentHomeJurisdiction but no compactConnectRegisteredEmailAddress should be left untouched.
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (5)
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#848
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py:111-117
Timestamp: 2025-06-17T19:05:36.255Z
Learning: In CompactConnect PR #848, the user landonshumway-ia decided to leave timezone handling code in _should_allow_reregistration function as-is after testing in sandbox environment confirmed it works correctly. The user's reasoning was that reregistration is an edge case, the code has been tested and verified, and AWS is unlikely to change behavior that would break many clients. This represents a pragmatic engineering decision based on testing and risk assessment.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#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.
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#824
File: backend/compact-connect/tests/app/test_event_listener.py:29-222
Timestamp: 2025-06-04T20:07:09.680Z
Learning: In the CompactConnect project, test methods that include property snapshots for different AWS resource configurations (like event listeners) should be kept decoupled rather than extracted into common helper methods, even if they appear similar. This avoids complex parameter passing and maintains test independence when snapshots may vary between configurations.
🧬 Code Graph Analysis (1)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (2)
backend/compact-connect/tests/smoke/smoke_common.py (4)
SmokeTestFailureException(13-17)call_provider_users_me_endpoint(225-233)get_provider_user_auth_headers_cached(163-171)get_user_tokens(132-160)backend/compact-connect/tests/smoke/config.py (2)
api_base_url(16-17)test_provider_user_password(52-53)
🪛 Flake8 (7.2.0)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
[error] 2-2: block comment should start with '# '
(E265)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (4)
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py (1)
22-192: Comprehensive smoke test implementation.The test function effectively covers the complete email update workflow from initiation through verification, authentication testing, and restoration. The manual verification approach with user input is appropriate for smoke testing in a sandbox environment.
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py (3)
110-116: Excellent verification code validation.The test properly validates both the length and numeric nature of the verification code, ensuring robust validation of the generated 4-digit code.
142-163: Good input validation approach.The test correctly validates that whitespace is trimmed while preserving email case, ensuring proper input sanitization without altering user expectations.
596-644: Critical rollback functionality well-tested.The rollback test ensures data consistency by verifying that Cognito changes are properly reverted when DynamoDB operations fail. This is essential for maintaining system integrity during email updates.
backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/nodejs/tests/lib/email/email-notification-service.test.ts
Outdated
Show resolved
Hide resolved
...ct/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
Show resolved
Hide resolved
...ct/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/practitioner_email_update_smoke_tests.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/provider-data-v1/handlers/provider_users.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py
Show resolved
Hide resolved
Providers need the ability to update their email addresses in the CompactConnect system. The system uses AWS Cognito with email as username and stores provider data in DynamoDB. Email addresses must stay synchronized between Cognito and DynamoDB while maintaining proper verification and audit trails.
This change implements a custom email verification system using a 4-digit code sent via our existing email notification service. When a provider requests to update to a new email, we store temporary verification data on the provider record, then update both Cognito and DynamoDB atomically upon successful verification. We also send a notification to the old email address after successful email change to make them aware the email has been updated.
Closes #872
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores