Whatsapp failed msg logging #1607
Conversation
WalkthroughThe changes in this pull request introduce modifications to the Changes
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: 2
🧹 Outside diff range and nitpick comments (7)
kairon/shared/chat/processor.py (2)
170-183: Minor documentation improvements neededThe method signature and documentation are well-structured, but consider these improvements:
- Add type hints in the docstring for better clarity
- Add examples of usage in the docstring
- Document possible exceptions that might be raised
@staticmethod def save_whatsapp_failed_messages(resp: Dict, bot: Text, recipient: Text, channel_type: Text, **kwargs): """ Logs failed WhatsApp message responses for debugging and tracking purposes. Args: - resp (Dict): The response dictionary containing error details. - bot (Text): The bot identifier. - recipient (Text): The recipient identifier. - channel_type (Text): The type of channel (e.g., WhatsApp). + resp (Dict): The response dictionary containing error details. Example: {"error": {"error_data": {"details": "Invalid number"}}} + bot (Text): The bot identifier. Example: "bot_123" + recipient (Text): The recipient identifier. Example: "+1234567890" + channel_type (Text): The type of channel (e.g., WhatsApp). + **kwargs: Additional keyword arguments including: + - json_message (Dict): The original message that failed to send Returns: - None + None: The method saves the log entry to the database. + + Raises: + mongoengine.errors.ValidationError: If required fields are missing or invalid """
170-201: Consider architectural improvements for logging consistencyThe new method shares similar functionality with
save_whatsapp_audit_log. Consider these architectural improvements:
- Create a common base method for WhatsApp logging
- Standardize error handling and logging patterns
- Consider using an enum for status types
Example approach:
from enum import Enum class WhatsAppMessageStatus(Enum): DELIVERED = "delivered" READ = "read" FAILED = "failed" CAPTURED = "captured" @staticmethod def _save_whatsapp_log( status: WhatsAppMessageStatus, data: Dict, bot: Text, recipient: Text, channel_type: Text, **kwargs ) -> None: """Common method for WhatsApp logging.""" # Common logging logic hereThis would help maintain consistency across different types of WhatsApp message logging and make the code more maintainable.
kairon/chat/handlers/channels/whatsapp.py (2)
241-245: Enhance security measures for message handlingConsider implementing additional security measures:
- Validate and sanitize the recipient_id format before processing
- Implement rate limiting for failed message logging
- Sanitize error messages before logging to prevent log injection
- Add audit logging for security-sensitive operations
Would you like me to provide a detailed implementation for these security enhancements?
241-245: Add comprehensive test coverageThe new error handling logic requires additional test coverage:
- Unit tests for various error scenarios:
- Missing assistant_id
- Various WhatsApp error responses
- Database logging failures
- Integration tests for WhatsApp client interaction
- Mocking strategy for external dependencies
Would you like me to help create the test cases?
tests/unit_test/chat/chat_test.py (3)
244-250: Remove duplicate assertion for message type.The test contains a duplicate assertion for the WhatsApp message type:
assert log['type'] == 'whatsapp' # Line 244 assert log['type'] == 'whatsapp' # Line 250Remove one of these duplicate assertions to maintain cleaner test code.
305-305: Remove unused variable.The variable
channel_typeis assigned but never used in the test method.Remove this unused variable assignment:
-channel_type = "whatsapp"🧰 Tools
🪛 Ruff (0.8.0)
305-305: Local variable
channel_typeis assigned to but never usedRemove assignment to unused variable
channel_type(F841)
317-323: Remove duplicate assertion for message type.The test contains a duplicate assertion for the WhatsApp message type:
assert log['type'] == 'whatsapp' # Line 317 assert log['type'] == 'whatsapp' # Line 323Remove one of these duplicate assertions to maintain cleaner test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
kairon/chat/handlers/channels/whatsapp.py(1 hunks)kairon/shared/chat/processor.py(1 hunks)tests/unit_test/chat/chat_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
tests/unit_test/chat/chat_test.py
305-305: Local variable channel_type is assigned to but never used
Remove assignment to unused variable channel_type
(F841)
🔇 Additional comments (3)
kairon/shared/chat/processor.py (1)
170-201: Verify logging patterns and model usage
Let's verify the consistency of logging patterns and ChannelLogs model usage across the codebase.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Implementation is consistent with logging patterns
The implementation of save_whatsapp_failed_messages follows the correct pattern for logging WhatsApp message failures:
- Uses the standard
ChannelLogsmodel with all required fields (type, status, data, message_id, bot, user) - Properly handles error details through the
failure_reasonfield - Consistent with test cases showing various message statuses (sent, delivered, read, failed)
- Follows the same pattern used in other WhatsApp message logging throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar logging patterns and ChannelLogs usage
# Find similar logging patterns
echo "Checking for similar logging patterns..."
rg -A 5 "ChannelLogs\(" --type py
# Check for other WhatsApp-related logging
echo "Checking for WhatsApp-related logging..."
rg -B 2 -A 2 "whatsapp.*log" --type py -i
# Find all status types used with ChannelLogs
echo "Checking ChannelLogs status types..."
rg "status=['\"].*['\"].*ChannelLogs" --type py
Length of output: 101334
Script:
#!/bin/bash
# Description: Check for ChannelLogs model definition and usage
# Find ChannelLogs model definition
echo "Checking ChannelLogs model definition..."
ast-grep --pattern 'class ChannelLogs($$$)' --lang python
# Find all ChannelLogs instantiations with status field
echo "Checking ChannelLogs instantiations with status..."
ast-grep --pattern 'ChannelLogs(
$$$
status=$_,
$$$
)' --lang python
# Find all WhatsApp message failure handling
echo "Checking WhatsApp message failure handling..."
rg -B 2 -A 2 "save_whatsapp.*failed" --type py
Length of output: 73250
Script:
#!/bin/bash
# Description: Check for ChannelLogs model definition and related code
# Find ChannelLogs model definition
echo "Checking ChannelLogs model definition..."
rg -A 10 "class ChannelLogs" --type py
# Find WhatsApp message status handling
echo "Checking WhatsApp message status handling..."
rg -B 2 -A 2 "status.*whatsapp" --type py -i
# Find all imports of ChannelLogs
echo "Checking ChannelLogs imports..."
rg "from.*ChannelLogs.*import" --type py
Length of output: 1196
kairon/chat/handlers/channels/whatsapp.py (1)
241-245: Verify consistent error handling patterns
Let's verify if similar error handling patterns exist in the codebase:
✅ Verification successful
Error handling implementation is consistent and complete
The verification confirms that the WhatsApp error handling implementation is consistent and well-integrated:
- The
save_whatsapp_failed_messagesmethod inChatDataProcessorproperly logs failed messages toChannelLogswith all necessary context (bot, recipient, channel type, error details) - The error handling pattern is consistent with other channel implementations and follows the established logging pattern using
ChannelLogs - The implementation includes proper error tracking with status, message IDs, and failure reasons
- Test coverage demonstrates comprehensive error scenarios and logging validation
The code follows a consistent pattern for error handling across the WhatsApp channel implementation and integrates well with the existing logging infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns in WhatsApp-related code
# Look for WhatsApp error handling patterns
echo "Searching for WhatsApp error handling patterns..."
rg -A 5 "whatsapp.*error" --type py
# Look for similar message logging patterns
echo "Searching for message logging patterns..."
ast-grep --pattern 'save_whatsapp_$_messages'
# Check for other channel implementations
echo "Checking other channel implementations..."
rg -A 5 "send_custom_json.*error" --type py
Length of output: 16937
Script:
#!/bin/bash
# Let's search for the implementation of save_whatsapp_failed_messages and similar error handling patterns
# Search for the implementation of save_whatsapp_failed_messages
echo "Searching for WhatsApp message saving implementations..."
ast-grep --pattern 'def save_whatsapp_failed_messages'
# Look for error handling in WhatsApp client
echo "Searching for WhatsApp client error handling..."
rg -A 5 "if.*error.*whatsapp" --type py
# Check for similar error handling in message processors
echo "Checking message processor error handling..."
rg -A 5 "ChatDataProcessor.*save" --type py
Length of output: 26178
Script:
#!/bin/bash
# Let's look for the implementation of save_whatsapp_failed_messages and related error handling
# Search for save_whatsapp_failed_messages implementation
echo "Searching for save_whatsapp_failed_messages implementation..."
rg -A 10 "def save_whatsapp_failed_messages" --type py
# Look for similar error handling in other channel handlers
echo "Searching for error handling in channel handlers..."
rg -A 5 "if.*resp.get\(.*error" --type py
# Check for ChannelLogs usage
echo "Checking ChannelLogs usage..."
rg -A 5 "ChannelLogs" --type py
Length of output: 46857
tests/unit_test/chat/chat_test.py (1)
201-340: LGTM! Comprehensive test coverage for WhatsApp message logging.
The test implementation effectively covers:
- Direct usage of the logging function
- Integration with WhatsApp bot's message sending
- Verification of all log fields
- Error scenarios with invalid message formats
🧰 Tools
🪛 Ruff (0.8.0)
305-305: Local variable channel_type is assigned to but never used
Remove assignment to unused variable channel_type
(F841)
kairon/shared/chat/processor.py
Outdated
| error = resp.get("error", {}) | ||
| message_id = f"failed-{channel_type}-{recipient}-{bot}" | ||
| user = "unknown_user" | ||
| failure_reason = error.get("error_data", {}).get("details") | ||
| logger.debug(f"WhatsApp message failed to send: {error}") | ||
|
|
||
| ChannelLogs( | ||
| type=channel_type, | ||
| status="failed", | ||
| data=resp, | ||
| failure_reason=failure_reason, | ||
| message_id=message_id, | ||
| user=user, | ||
| bot=bot, | ||
| recipient=recipient, | ||
| json_message=kwargs.get('json_message') | ||
| ).save() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in the implementation
The implementation has several areas that could be improved for better reliability and maintainability:
- The hardcoded "unknown_user" should be replaced with a proper user identifier from kwargs or system constants
- The message ID format might not guarantee uniqueness
- Missing validation for required fields
- No error handling for database operations
- No sanitization of the failure reason
Consider applying these improvements:
- error = resp.get("error", {})
- message_id = f"failed-{channel_type}-{recipient}-{bot}"
- user = "unknown_user"
- failure_reason = error.get("error_data", {}).get("details")
- logger.debug(f"WhatsApp message failed to send: {error}")
+ error = resp.get("error", {})
+ timestamp = datetime.utcnow().strftime("%Y%m%d%H%M%S")
+ message_id = f"failed-{channel_type}-{recipient}-{bot}-{timestamp}"
+ user = kwargs.get('user', 'system')
+
+ error_data = error.get("error_data", {})
+ failure_reason = error_data.get("details")
+ if failure_reason:
+ failure_reason = str(failure_reason)[:500] # Limit length for storage
+
+ logger.debug(f"WhatsApp message failed to send: {error}")
+
+ try:
+ if not all([bot, recipient, channel_type]):
+ raise ValueError("Missing required fields: bot, recipient, or channel_type")
- ChannelLogs(
+ log_entry = ChannelLogs(
type=channel_type,
status="failed",
data=resp,
@@ -199,3 +208,8 @@
recipient=recipient,
json_message=kwargs.get('json_message')
- ).save()
+ )
+ log_entry.save()
+ except Exception as e:
+ logger.error(f"Failed to save WhatsApp error log: {str(e)}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error = resp.get("error", {}) | |
| message_id = f"failed-{channel_type}-{recipient}-{bot}" | |
| user = "unknown_user" | |
| failure_reason = error.get("error_data", {}).get("details") | |
| logger.debug(f"WhatsApp message failed to send: {error}") | |
| ChannelLogs( | |
| type=channel_type, | |
| status="failed", | |
| data=resp, | |
| failure_reason=failure_reason, | |
| message_id=message_id, | |
| user=user, | |
| bot=bot, | |
| recipient=recipient, | |
| json_message=kwargs.get('json_message') | |
| ).save() | |
| error = resp.get("error", {}) | |
| timestamp = datetime.utcnow().strftime("%Y%m%d%H%M%S") | |
| message_id = f"failed-{channel_type}-{recipient}-{bot}-{timestamp}" | |
| user = kwargs.get('user', 'system') | |
| error_data = error.get("error_data", {}) | |
| failure_reason = error_data.get("details") | |
| if failure_reason: | |
| failure_reason = str(failure_reason)[:500] # Limit length for storage | |
| logger.debug(f"WhatsApp message failed to send: {error}") | |
| try: | |
| if not all([bot, recipient, channel_type]): | |
| raise ValueError("Missing required fields: bot, recipient, or channel_type") | |
| log_entry = ChannelLogs( | |
| type=channel_type, | |
| status="failed", | |
| data=resp, | |
| failure_reason=failure_reason, | |
| message_id=message_id, | |
| user=user, | |
| bot=bot, | |
| recipient=recipient, | |
| json_message=kwargs.get('json_message') | |
| ) | |
| log_entry.save() | |
| except Exception as e: | |
| logger.error(f"Failed to save WhatsApp error log: {str(e)}") | |
| raise |
| resp = self.whatsapp_client.send(response, recipient_id, messaging_type) | ||
| if resp.get("error"): | ||
| bot = kwargs.get("assistant_id") | ||
| ChatDataProcessor.save_whatsapp_failed_messages(resp, bot, recipient_id, ChannelTypes.WHATSAPP.value, | ||
| json_message=json_message) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and improve error handling
The error handling implementation could be enhanced in several ways:
- Add type hints and improve error handling:
- resp = self.whatsapp_client.send(response, recipient_id, messaging_type)
- if resp.get("error"):
- bot = kwargs.get("assistant_id")
- ChatDataProcessor.save_whatsapp_failed_messages(resp, bot, recipient_id, ChannelTypes.WHATSAPP.value,
- json_message=json_message)
+ resp: Dict[str, Any] = self.whatsapp_client.send(response, recipient_id, messaging_type)
+ if resp.get("error"):
+ bot = kwargs.get("assistant_id")
+ if not bot:
+ logger.error("Missing assistant_id in kwargs for failed message logging")
+ return
+ logger.error(f"WhatsApp message failed: {resp.get('error')}")
+ try:
+ ChatDataProcessor.save_whatsapp_failed_messages(
+ resp, bot, recipient_id, ChannelTypes.WHATSAPP.value,
+ json_message=json_message
+ )
+ except Exception as e:
+ logger.error(f"Failed to log WhatsApp error: {str(e)}")- Consider adding validation for the response structure:
if not isinstance(resp, dict):
logger.error(f"Unexpected response type from WhatsApp client: {type(resp)}")
return
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
kairon/chat/handlers/channels/whatsapp.py (1)
241-254: Consider adding performance monitoringTo track and optimize the performance of WhatsApp message handling:
Add metrics collection for:
- Message send latency
- Error rates
- Retry attempts
- Database operation latency
Consider implementing circuit breaker pattern for external calls to prevent cascading failures.
Would you like me to provide an implementation example?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
kairon/chat/handlers/channels/whatsapp.py(1 hunks)
🔇 Additional comments (1)
kairon/chat/handlers/channels/whatsapp.py (1)
241-254: Verify error handling coverage
Let's verify the error handling implementation across the codebase:
✅ Verification successful
Based on the shell script results, I can see that the error handling implementation in the WhatsApp handler is comprehensive and follows good practices:
- The code logs errors at appropriate severity levels using logger.error()
- Failed message details are saved using ChatDataProcessor.save_whatsapp_failed_messages()
- Error responses from the WhatsApp API are properly checked and handled
- Exception handling is in place for the save operation itself
Error handling is properly implemented
The code follows a robust error handling pattern:
- Checks for error in WhatsApp API response
- Logs error details at appropriate severity level
- Persists failed message details for debugging/monitoring
- Has exception handling for the persistence operation itself
- Includes relevant context like bot ID in error logs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in WhatsApp related code
# Check for error handling patterns in WhatsApp related files
echo "Checking error handling patterns in WhatsApp related files..."
rg -A 5 "WhatsappBot|WhatsappHandler|Whatsapp" --type py
# Check for logging patterns
echo "Checking logging patterns..."
rg "logger\.(error|warning|info|debug)" --type py
# Check for retry mechanisms
echo "Checking retry mechanisms..."
rg "attempt|retry|backoff" --type py
# Check for error response handling
echo "Checking error response handling..."
rg "get\(['\"]error['\"]" --type py
Length of output: 102220
| resp = self.whatsapp_client.send(response, recipient_id, messaging_type) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and response validation
The response handling could be improved with proper type hints and validation:
- resp = self.whatsapp_client.send(response, recipient_id, messaging_type)
+ resp: Dict[str, Any] = self.whatsapp_client.send(response, recipient_id, messaging_type)
+ if not isinstance(resp, dict):
+ logger.error(f"Unexpected response type from WhatsApp client: {type(resp)}")
+ return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp = self.whatsapp_client.send(response, recipient_id, messaging_type) | |
| resp: Dict[str, Any] = self.whatsapp_client.send(response, recipient_id, messaging_type) | |
| if not isinstance(resp, dict): | |
| logger.error(f"Unexpected response type from WhatsApp client: {type(resp)}") | |
| return | |
| if resp.get("error"): | ||
| bot = kwargs.get("assistant_id") | ||
| if not bot: | ||
| logger.error("Missing assistant_id in kwargs for failed message logging") | ||
| return | ||
| logger.error(f"WhatsApp message failed: {resp.get('error')}") | ||
| try: | ||
| ChatDataProcessor.save_whatsapp_failed_messages( | ||
| resp, bot, recipient_id, ChannelTypes.WHATSAPP.value, | ||
| json_message=json_message | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging security
The error handling implementation could be improved in several ways:
- Add retry mechanism for transient failures
- Sanitize sensitive data in logs
- Add structured logging with correlation IDs
- if resp.get("error"):
- bot = kwargs.get("assistant_id")
- if not bot:
- logger.error("Missing assistant_id in kwargs for failed message logging")
- return
- logger.error(f"WhatsApp message failed: {resp.get('error')}")
- try:
- ChatDataProcessor.save_whatsapp_failed_messages(
- resp, bot, recipient_id, ChannelTypes.WHATSAPP.value,
- json_message=json_message
- )
- except Exception as e:
- logger.error(f"Failed to log WhatsApp error: {str(e)}")
+ if resp.get("error"):
+ bot = kwargs.get("assistant_id")
+ if not bot:
+ logger.error("Missing assistant_id in kwargs for failed message logging",
+ extra={"correlation_id": Utility.generate_correlation_id()})
+ return
+
+ error_context = {
+ "correlation_id": Utility.generate_correlation_id(),
+ "error_code": resp.get("error", {}).get("code"),
+ "error_type": resp.get("error", {}).get("type"),
+ "recipient_id": Utility.mask_sensitive_data(recipient_id)
+ }
+ logger.error("WhatsApp message failed", extra=error_context)
+
+ try:
+ for attempt in range(3): # Retry up to 3 times
+ try:
+ ChatDataProcessor.save_whatsapp_failed_messages(
+ resp, bot, recipient_id, ChannelTypes.WHATSAPP.value,
+ json_message=json_message
+ )
+ break
+ except Exception as e:
+ if attempt == 2: # Last attempt
+ logger.error(f"Failed to log WhatsApp error after 3 attempts",
+ extra={"correlation_id": error_context["correlation_id"],
+ "error": str(e)})
+ else:
+ time.sleep(1 * (attempt + 1)) # Exponential backoff
+ except Exception as e:
+ logger.error("Failed to log WhatsApp error",
+ extra={"correlation_id": error_context["correlation_id"],
+ "error": str(e)})Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
kairon/chat/handlers/channels/clients/whatsapp/cloud.py (1)
49-49: Add documentation and type hints for the metadata parameter.Please update the following:
- Add metadata to the docstring's @optional section with its purpose
- Add type hints for consistency with other parameters
Apply this diff:
def __init__(self, access_token, **kwargs): """ @required: access_token @optional: session api_version app_secret + metadata: Additional context for message tracking and logging """ - self.metadata = kwargs.get("metadata") + self.metadata: dict = kwargs.get("metadata")kairon/chat/handlers/channels/whatsapp.py (1)
161-161: Add metadata validationConsider adding validation before assigning metadata to prevent potential issues if metadata is None.
- self.client.metadata = metadata + if metadata is not None: + self.client.metadata = metadata + else: + logger.warning("No metadata provided for WhatsApp client") + self.client.metadata = {}tests/unit_test/chat/chat_test.py (4)
246-246: Remove debug print statement.The print statement is not needed for the test and should be removed.
- print(log)
201-270: Add test data cleanup.Consider adding test data cleanup in a teardown method to ensure test isolation.
Add this cleanup code in a teardown method:
def teardown_method(self): from kairon.shared.chat.data_objects import ChannelLogs ChannelLogs.objects(bot="5e564fbcdcf0d5fad89e3abd").delete()
309-309: Remove unused variable.The
channel_typevariable is assigned but never used.- channel_type = "whatsapp"🧰 Tools
🪛 Ruff (0.8.0)
309-309: Local variable
channel_typeis assigned to but never usedRemove assignment to unused variable
channel_type(F841)
271-374: Add test data cleanup.Consider adding test data cleanup in a teardown method to ensure test isolation.
Add this cleanup code in a teardown method:
def teardown_method(self): from kairon.shared.chat.data_objects import ChannelLogs ChannelLogs.objects(bot="5e564fbcdcf0d5fad89e3abcd").delete()🧰 Tools
🪛 Ruff (0.8.0)
309-309: Local variable
channel_typeis assigned to but never usedRemove assignment to unused variable
channel_type(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
kairon/chat/handlers/channels/clients/whatsapp/cloud.py(1 hunks)kairon/chat/handlers/channels/whatsapp.py(2 hunks)kairon/shared/chat/processor.py(1 hunks)tests/unit_test/chat/chat_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kairon/shared/chat/processor.py
🧰 Additional context used
🪛 Ruff (0.8.0)
tests/unit_test/chat/chat_test.py
309-309: Local variable channel_type is assigned to but never used
Remove assignment to unused variable channel_type
(F841)
🔇 Additional comments (3)
kairon/chat/handlers/channels/clients/whatsapp/cloud.py (1)
49-49: Verify if metadata parameter is necessary at this stage.
The metadata parameter is added but not utilized within this class. While the AI summary suggests it's for future use in WhatsappBot, consider:
- Whether it should be added only when the actual functionality is implemented
- If it's needed now, document its intended future use in comments
Let's check if this parameter is actually used in WhatsappBot:
✅ Verification successful
Metadata parameter is necessary and actively used
The metadata parameter is extensively used throughout the WhatsApp integration:
- It's passed through multiple handler methods and updated with channel-specific information
- Used for message processing, webhook handling, and error logging
- Contains critical information like phone numbers, message IDs, and channel types
- The client's metadata is accessed in WhatsappBot for sending messages and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for metadata usage in WhatsappBot
rg -l "metadata" kairon/chat/handlers/channels/whatsapp.py
Length of output: 100
Script:
#!/bin/bash
# Search for actual usage of metadata in WhatsappBot
rg -A 5 "metadata" kairon/chat/handlers/channels/whatsapp.py
Length of output: 4971
kairon/chat/handlers/channels/whatsapp.py (1)
242-243: Add type hints and response validation
This was previously suggested in past reviews and is still applicable. Add type hints and validate the response structure.
tests/unit_test/chat/chat_test.py (1)
201-374: Well-structured tests with good coverage!
The tests thoroughly verify the WhatsApp failed message logging functionality through both direct API testing and integration testing. They validate all required fields in the log entries and handle error scenarios appropriately.
🧰 Tools
🪛 Ruff (0.8.0)
309-309: Local variable channel_type is assigned to but never used
Remove assignment to unused variable channel_type
(F841)
| if resp.get("error"): | ||
| bot = kwargs.get("assistant_id") | ||
| message_id = self.whatsapp_client.metadata.get("id") | ||
| user = self.whatsapp_client.metadata.get("display_phone_number") | ||
| if not bot: | ||
| logger.error("Missing assistant_id in kwargs for failed message logging") | ||
| return | ||
| logger.error(f"WhatsApp message failed: {resp.get('error')}") | ||
| try: | ||
| ChatDataProcessor.save_whatsapp_failed_messages( | ||
| resp, bot, recipient_id, ChannelTypes.WHATSAPP.value, | ||
| json_message=json_message, message_id=message_id, user=user, | ||
| metadata=self.whatsapp_client.metadata | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Failed to log WhatsApp error: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with retry mechanism
While the error handling is comprehensive, consider adding a retry mechanism for transient failures and handling specific error types differently.
- if resp.get("error"):
+ max_retries = 3
+ retry_count = 0
+ while retry_count < max_retries:
+ if resp.get("error"):
+ error_type = resp.get("error", {}).get("type")
+ error_code = resp.get("error", {}).get("code")
+
+ # Don't retry for permanent errors
+ if error_type in ["permanent_error", "invalid_parameter"]:
+ break
+
+ if retry_count < max_retries - 1:
+ retry_count += 1
+ await asyncio.sleep(2 ** retry_count) # Exponential backoff
+ resp = self.whatsapp_client.send(response, recipient_id, messaging_type)
+ continue
+
bot = kwargs.get("assistant_id")
message_id = self.whatsapp_client.metadata.get("id")
user = self.whatsapp_client.metadata.get("display_phone_number")
if not bot:
logger.error("Missing assistant_id in kwargs for failed message logging")
return
- logger.error(f"WhatsApp message failed: {resp.get('error')}")
+ logger.error(
+ "WhatsApp message failed",
+ extra={
+ "error": resp.get("error"),
+ "retry_count": retry_count,
+ "recipient_id": recipient_id,
+ "message_type": messaging_type
+ }
+ )
try:
ChatDataProcessor.save_whatsapp_failed_messages(
resp, bot, recipient_id, ChannelTypes.WHATSAPP.value,
json_message=json_message, message_id=message_id, user=user,
metadata=self.whatsapp_client.metadata
)
except Exception as e:
- logger.error(f"Failed to log WhatsApp error: {str(e)}")
+ logger.error(
+ "Failed to log WhatsApp error",
+ extra={
+ "error": str(e),
+ "recipient_id": recipient_id,
+ "message_type": messaging_type
+ }
+ )
+ breakCommittable suggestion skipped: line range outside the PR's diff.
Added code for logging whatsapp failed messages and added test cases accordingly.
Summary by CodeRabbit
New Features
Tests