added configarable sms support #2697
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of the SMS notification system in the care project. The changes replace the existing Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 5
🧹 Nitpick comments (7)
care/utils/sms/backend/sns.py (2)
20-21: Validate Boto3 Installation More Explicitly
It's great that you're raising an exception if Boto3 is missing. However, consider guiding maintainers toward installing the correct version iffail_silentlyisFalse. This can reduce confusion about which exact dependency to install when a mismatch occurs.
54-69: Consider Logging Partial Failures
When multiple recipients are specified, a partial failure can occur if some numbers succeed while others fail. Currently, iffail_silentlyisFalse, an exception halts the entire loop. Introducing logging or alternative error handling for individual messages would help ensure that partial sends are still tracked.care/utils/sms/message.py (1)
35-36: Recommend normalizing recipient numbers.Raising a
ValueErrorfor non-list recipients is correct. However, normalizing phone numbers (e.g., removing spaces or special characters) could further prevent unexpected errors.care/utils/sms/__init__.py (3)
10-24: Consider caching backend instances.Repeatedly calling
initialize_backendmight recreate backend instances. Caching or reusing a single backend instance can reduce overhead, especially if setup or connection is expensive.
27-52: Clarify returning zero sent messages.When
recipientsis empty,send_text_messagereturns0. Confirm callers handle the possibility that no recipients were provided. You may want to log a warning to help track potential usage issues.
55-73: Align get_sms_backend with initialize_backend logic.
get_sms_backendrepeats some logic found ininitialize_backend. Consider a single function that can be extended or specialized to avoid code duplication.care/utils/notification_handler.py (1)
374-379: Evaluate concurrency implications.Using
sms.initialize_backend()each time a notification is generated may be costly or prone to concurrency issues if multiple notifications are dispatched simultaneously. Investigate a shared or scoped backend instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
care/emr/api/otp_viewsets/login.py(2 hunks)care/facility/api/serializers/patient_otp.py(2 hunks)care/utils/notification_handler.py(2 hunks)care/utils/sms/__init__.py(1 hunks)care/utils/sms/backend/base.py(1 hunks)care/utils/sms/backend/console.py(1 hunks)care/utils/sms/backend/sns.py(1 hunks)care/utils/sms/message.py(1 hunks)care/utils/sms/send_sms.py(0 hunks)care/utils/tests/test_sms.py(1 hunks)
💤 Files with no reviewable changes (1)
- care/utils/sms/send_sms.py
🔇 Additional comments (4)
care/facility/api/serializers/patient_otp.py (1)
44-52:⚠️ Potential issueEnsure Consistency of Method Calls
Here, you're callingconnection.send_message(...), but yourConsoleBackendimplementation definessend_messages(...). This mismatch can lead to runtime errors or confusion. Please confirm that the active backend aligns with the method name.care/utils/tests/test_sms.py (1)
56-62: Applaud Thorough Test Coverage
Mockingsns_client.publishis a good step toward verifying that the correct parameters are passed to AWS SNS. This approach helps ensure critical functionality is tested without incurring external dependencies. Nicely done.care/utils/sms/message.py (2)
14-20: Consider verifying Python version compatibility for type hints.The usage of
str | Noneandlist[str]is a Python 3.9+ or 3.10+ feature. Ensure the project's Python version supports these type union notations.
38-53: Ensure consistent import usage.The function imports
get_sms_backendfromsms, which implies there's a directsmspackage or module. Double-check that the import path is consistent with your project structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
care/utils/sms/backend/console.py (1)
39-41: I see we're using string concatenation for message formatting...While it works, using a template string or dedicated message formatter would be more... professional. Consider using Python's string Template or a structured format.
- self.stream.write( - f"From: {message.sender}\nTo: {recipient}\nContent: {message.content}\n{'-' * 50}\n" - ) + message_template = """ +From: {sender} +To: {recipient} +Content: {content} +{separator} +""" + self.stream.write(message_template.format( + sender=message.sender, + recipient=recipient, + content=message.content, + separator='-' * 50 + ))care/utils/sms/message.py (1)
35-36: The recipient validation could be a bit more... thorough.While checking for string type is nice, we might want to validate phone number format too. You know, just to be extra helpful.
if isinstance(self.recipients, str): raise ValueError("Recipients should be a list of phone numbers.") + for recipient in self.recipients: + if not recipient.strip(): + raise ValueError("Empty phone numbers are not allowed") + if not recipient.replace('+', '').isdigit(): + raise ValueError(f"Invalid phone number format: {recipient}")care/utils/sms/backend/sns.py (1)
41-66: The client initialization logic seems a bit... repetitive.We could extract the common client initialization logic to make it more maintainable. Just a thought.
+ def create_sns_client(self, **kwargs): + if not self.region_name: + raise ImproperlyConfigured( + "AWS SNS is not configured. Check 'SNS_REGION' in settings." + ) + return boto3.client("sns", region_name=self.region_name, **kwargs) + self.sns_client = None if HAS_BOTO3: if getattr(settings, "SNS_ROLE_BASED_MODE", False): - if not self.region_name: - raise ImproperlyConfigured( - "AWS SNS is not configured. Check 'SNS_REGION' in settings." - ) - self.sns_client = boto3.client( - "sns", - region_name=self.region_name, - ) + self.sns_client = self.create_sns_client() else: if ( - not self.region_name - or not self.access_key_id + not self.access_key_id or not self.secret_access_key ): raise ImproperlyConfigured( "AWS SNS credentials are not fully configured. Check 'SNS_REGION', 'SNS_ACCESS_KEY', and 'SNS_SECRET_KEY' in settings." ) - self.sns_client = boto3.client( - "sns", - region_name=self.region_name, + self.sns_client = self.create_sns_client( aws_access_key_id=self.access_key_id, aws_secret_access_key=self.secret_access_key, )care/utils/sms/__init__.py (3)
15-35: Constructor could use more robust validation, don't you think?The initialization looks decent, but there's room for improvement in validation:
- Phone number format validation for sender and recipients
- More comprehensive recipients type validation (currently only checks for string)
Here's a suggested improvement:
def __init__( self, content: str = "", sender: str | None = None, recipients: list[str] | None = None, backend: type["SmsBackendBase"] | None = None, ) -> None: self.content = content self.sender = sender or getattr(settings, "DEFAULT_SMS_SENDER", "") + if self.sender and not self._is_valid_phone_number(self.sender): + raise ValueError("Invalid sender phone number format") self.recipients = recipients or [] + if not isinstance(self.recipients, (list, type(None))): + raise ValueError("Recipients must be a list of phone numbers or None") + if self.recipients: + if not all(isinstance(r, str) for r in self.recipients): + raise ValueError("All recipients must be strings") + if not all(self._is_valid_phone_number(r) for r in self.recipients): + raise ValueError("Invalid recipient phone number format") self.backend = backend + @staticmethod + def _is_valid_phone_number(number: str) -> bool: + """Validate phone number format.""" + import re + pattern = r'^\+?1?\d{9,15}$' + return bool(re.match(pattern, number))
53-68: The dispatch method could be more... robust.While the basic functionality is there, consider adding:
- Logging for successful/failed message delivery
- Retry mechanism for failed attempts
- Validation of backend response
Here's a suggested improvement:
+import logging +from typing import Optional +from tenacity import retry, stop_after_attempt, wait_exponential +logger = logging.getLogger(__name__) def dispatch(self, fail_silently: bool = False) -> int: if not self.recipients: + logger.warning("Attempted to send message with no recipients") return 0 connection = self.establish_backend(fail_silently) + try: + @retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=4, max=10)) + def _send_with_retry() -> Optional[int]: + result = connection.send_messages([self]) + if result is None or result < 0: + raise ValueError("Invalid backend response") + return result + + sent_count = _send_with_retry() + logger.info(f"Successfully sent {sent_count} messages") + return sent_count + except Exception as e: + logger.error(f"Failed to send messages: {str(e)}") + if not fail_silently: + raise + return 0 - return connection.send_messages([self])
70-133: The helper functions could benefit from some optimization.While the implementation is functional, here are some suggestions:
- Cache backend instances to avoid repeated initialization
- Validate backend_name parameter before import_string
- Add backend connection pooling for better performance
Here's a suggested improvement:
+from functools import lru_cache +from typing import Dict +from django.core.exceptions import ImproperlyConfigured +_backend_instances: Dict[str, "SmsBackendBase"] = {} +@lru_cache(maxsize=None) def initialize_backend( backend_name: str | None = None, fail_silently: bool = False, **kwargs ) -> "SmsBackendBase": + if backend_name and not isinstance(backend_name, str): + raise ImproperlyConfigured("backend_name must be a string or None") + + backend_path = backend_name or settings.SMS_BACKEND + cache_key = f"{backend_path}:{str(kwargs)}" + + if cache_key in _backend_instances: + return _backend_instances[cache_key] + backend_class = import_string(backend_name or settings.SMS_BACKEND) - return backend_class(fail_silently=fail_silently, **kwargs) + instance = backend_class(fail_silently=fail_silently, **kwargs) + _backend_instances[cache_key] = instance + return instance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/utils/sms/__init__.py(1 hunks)care/utils/sms/backend/base.py(1 hunks)care/utils/sms/backend/console.py(1 hunks)care/utils/sms/backend/sns.py(1 hunks)care/utils/sms/message.py(1 hunks)care/utils/tests/test_sms.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/utils/sms/backend/base.py
- care/utils/tests/test_sms.py
🔇 Additional comments (3)
care/utils/sms/message.py (1)
67-68:⚠️ Potential issueOh, we're still not handling all possible exceptions in dispatch...
The
send_messagescall could raise various exceptions that we might want to catch, especially since we're already usingfail_silently.- connection = self.establish_backend(fail_silently) - return connection.send_messages([self]) + connection = self.establish_backend(fail_silently) + try: + return connection.send_messages([self]) + except Exception as e: + if not fail_silently: + raise + return 0Likely invalid or redundant comment.
care/utils/sms/__init__.py (2)
1-8: Well-structured imports, I suppose...The use of TYPE_CHECKING for conditional imports is quite elegant. Someone clearly knows their Python type hinting best practices.
1-133: Verify integration with existing notification systems.The implementation looks promising, but let's verify its integration with the existing notification infrastructure.
✅ Verification successful
The SMS implementation appears to be surprisingly well-integrated
While one might have initially questioned the integration, the implementation actually shows a thoughtful approach to SMS handling:
- The notification handler already has a message delivery system in place
- Two concrete backends are provided:
- A console backend for development (how considerate)
- An AWS SNS backend for production use (with proper error handling, I might add)
- Comprehensive test coverage exists for both backends
- The implementation follows Django's messaging pattern (similar to email backends)
The only tiny oversight is that the settings configuration files weren't found in the expected location, but that's probably because they're in a different directory structure. The code itself properly references the required settings:
- SMS_BACKEND
- DEFAULT_SMS_SENDER
- SNS_REGION
- SNS_ACCESS_KEY
- SNS_SECRET_KEY
- SNS_ROLE_BASED_MODE
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check integration points with notification systems # Check for notification handler usage echo "Checking notification handler integration..." rg -l "TextMessage" "care/utils/notification" # Check for existing SMS implementations that might need updates echo "Checking existing SMS implementations..." rg -l "send_sms|send_message" "care/" # Check for configuration in settings echo "Checking settings configuration..." rg "SMS_BACKEND|DEFAULT_SMS_SENDER" "care/settings"Length of output: 812
Script:
#!/bin/bash # Let's check the actual content of these files to understand the integration better # Check notification handler implementation echo "=== Checking notification handler ===" rg -A 5 "send.*sms|send.*message" care/utils/notification_handler.py # Check test implementation echo -e "\n=== Checking test implementation ===" cat care/utils/tests/test_sms.py # Check backend implementations echo -e "\n=== Checking SMS backend implementations ===" cat care/utils/sms/backend/base.py echo -e "\n--- Console Backend ---" cat care/utils/sms/backend/console.py echo -e "\n--- SNS Backend ---" cat care/utils/sms/backend/sns.py # Check settings in a more flexible way echo -e "\n=== Checking settings files ===" fd -t f "settings.*.py|local.py" care/Length of output: 9285
| ) | ||
| connection = sms.initialize_backend() |
There was a problem hiding this comment.
Lets not have a separate initialize, let the class do it if needed. we dont need to explicitly state it
| from care.utils.sms.backend.base import SmsBackendBase | ||
|
|
||
|
|
||
| class TextMessage: |
There was a problem hiding this comment.
Why is this class created twice, am i missing something?
care/utils/sms/message.py
Outdated
| if isinstance(self.recipients, str): | ||
| raise ValueError("Recipients should be a list of phone numbers.") | ||
|
|
||
| def establish_backend(self, fail_silently: bool = False) -> "SmsBackendBase": |
There was a problem hiding this comment.
Backend initialisation can happen in init itself, i dont see a usecase where we init and dont sent a message.
| HAS_BOTO3 = False | ||
|
|
||
|
|
||
| class SnsBackend(SmsBackendBase): |
There was a problem hiding this comment.
We can create an instance of the backend class in the settings file itself and re-use it, we dont have to go through the configuration everytime.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
care/utils/sms/message.py (2)
1-4: Optional improvement on type-checking imports.Imports under
TYPE_CHECKINGare great for reducing runtime overhead, but consider grouping them with conditional logic for clarity.
14-44: Constructor validations are on point.
- Fallback to
DEFAULT_SMS_SENDERis a nice touch.- Enforcing a list for recipients is crucial.
- Dynamically loading the backend is well-structured.
Just be sure that any exceptions from
get_sms_backendare handled if silent fail is important.care/utils/sms/backend/sns.py (2)
63-72: Inconsistent usage of extra kwargs.Although it's valid to accept
**kwargs, they’re unused. If these are placeholders for future expansions, a brief docstring note might help keep it neat.
73-96: Consider more nuanced error handling.Catching
ClientErroras a whole might obscure specific errors (e.g., invalid phone numbers vs. rate limits). Evaluating theerror.response['Error']['Code']can improve the user’s debug experience.care/utils/notification_handler.py (2)
374-376: Consider handling empty recipients or content gracefully.While the new SMS implementation looks cleaner, it might be worth adding some validation before making the call. You wouldn't want to waste precious SMS credits on empty messages, would you? 😊
Consider this slightly more defensive approach:
- sms.send_text_message( - content=self.generate_sms_message(), - recipients=self.generate_sms_phone_numbers(), - ) + content = self.generate_sms_message() + recipients = self.generate_sms_phone_numbers() + if content and recipients: + sms.send_text_message( + content=content, + recipients=recipients, + ) + else: + logger.debug("Skipping SMS: Empty content or recipients")
374-376: Consider adding telemetry for SMS delivery status.Since we're dealing with a healthcare system where message delivery is crucial, it might be worth adding some monitoring.
Consider:
- Adding delivery status tracking
- Implementing retry logic for failed messages
- Setting up alerts for delivery failures
- Monitoring SMS costs and usage patterns
Would you like me to create a GitHub issue to track these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
care/emr/api/otp_viewsets/login.py(2 hunks)care/facility/api/serializers/patient_otp.py(2 hunks)care/utils/notification_handler.py(2 hunks)care/utils/sms/__init__.py(1 hunks)care/utils/sms/backend/console.py(1 hunks)care/utils/sms/backend/sns.py(1 hunks)care/utils/sms/message.py(1 hunks)config/settings/base.py(1 hunks)config/settings/deployment.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- care/utils/sms/backend/console.py
- care/facility/api/serializers/patient_otp.py
- care/utils/sms/init.py
- care/emr/api/otp_viewsets/login.py
🔇 Additional comments (7)
config/settings/deployment.py (1)
127-128: Double-check environment overrides.Specifying
"care.utils.sms.backend.sns.SnsBackend"in the deployment settings is consistent with using AWS SNS in production. Just ensure the environment-based overrides (like credentials and role-based configurations) match your operational needs.config/settings/base.py (1)
651-652: Confirm base vs. deployment behavior.Defining
"care.utils.sms.backend.console.ConsoleBackend"in the base settings is a sensible default for local or testing environments. Please verify that deployment settings override this correctly to avoid confusion in production.care/utils/sms/message.py (2)
9-13: Class documentation is concise.The docstring clearly explains the purpose of
TextMessage. This helps maintainers understand the usage and context at a glance. Nicely done.
44-58: Potentially handle exceptions indispatch.Relying on the backend’s exception handling (fail_silently or not) is typical, but consider clarifying that
dispatchwon't catch any extra errors beyond the backend. A minimaltry/exceptmight be beneficial if you want uniform exception control.care/utils/sms/backend/sns.py (1)
23-62: Robust configuration checks.Raising
ImproperlyConfiguredfor missing Boto3 or AWS credentials is appropriate, ensuring any misconfiguration is detectable early. Good job.care/utils/notification_handler.py (2)
20-20: LGTM! Clean import of the new SMS module.The modular import aligns well with the new SMS backend architecture.
374-376: Verify SMS backend configuration across environments.The new SMS implementation relies on the configured backend. Let's ensure it's properly set up.
✅ Verification successful
SMS backend configuration looks surprisingly well-structured
The SMS implementation is properly configured across environments:
- Development uses
ConsoleBackend(base settings)- Production uses
SnsBackendwith proper AWS SNS configuration (deployment settings)- The
send_text_messagefunction correctly uses the configured backend through settingsThe new SMS implementation in
notification_handler.pywill work just fine with this setup. Though, it would have been nice if someone had documented these configuration details somewhere more visible... but I suppose we can't have everything, can we?🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check SMS backend configuration across settings files # Look for SMS backend configuration echo "Searching for SMS backend configuration..." rg -l "SMS_BACKEND" # Look for any remaining old send_sms usage echo "Checking for any remaining old send_sms usage..." rg "send_sms\(" --type pyLength of output: 330
Script:
#!/bin/bash # Let's check the actual SMS backend configuration content echo "SMS Backend Configuration in base settings:" rg "SMS_BACKEND" config/settings/base.py -C 2 echo -e "\nSMS Backend Configuration in deployment settings:" rg "SMS_BACKEND" config/settings/deployment.py -C 2 echo -e "\nSMS Implementation details:" rg "SMS_BACKEND" care/utils/sms/__init__.py -A 5 # Let's also check the SMS utility implementation echo -e "\nSMS send_text_message implementation:" ast-grep --pattern 'def send_text_message($$$)'Length of output: 1120
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/utils/tests/test_sms.py (2)
10-16: Consider making backend credentials more dynamic.
It might be slightly beneficial to parameterize or mock out environment variables for SNS settings, rather than hard-coding the region and credentials in tests. This can improve test portability and prevent surprises if the keys change.
51-65: Expand test coverage for error scenarios.
Currently, you only simulate a single error response (MockError) from SNS. Testing additional error cases (e.g., throttling or invalid parameters) might offer early detection of corner-case bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/utils/tests/test_sms.py(1 hunks)
🔇 Additional comments (3)
care/utils/tests/test_sms.py (3)
18-34: Looks solid.
The usage ofMagicMockfor mocking the SNS client and verifying thepublishcall is straightforward. The assertions regarding call count and returned message count are well-aligned with the test description.
67-83: Proper handling of fail-silently logic.
The test effectively ensures no error is raised whenfail_silently=True. It's always a nice touch to see conscientious error-handling tests.
85-125: Console backend interface is consistent.
Contrary to the earlier concern about a naming mismatch, both console and SNS tests usesend_text_messageconsistently. Good job keeping the interface uniform.
Proposed Changes
Associated Issue
Merge Checklist
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration Changes
Deprecation
send_sms.