Broadcast template fix #1592
Broadcast template fix #1592
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing the functionality and error handling of 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: 5
🧹 Outside diff range and nitpick comments (7)
kairon/events/definitions/message_broadcast.py (2)
Line range hint
39-44: Add docstring to validate_retry_broadcast method.The method logic is correct, but it would benefit from proper documentation explaining its purpose, parameters, and possible exceptions.
Add a docstring like this:
def validate_retry_broadcast(self, event_id: Text): + """ + Validates if the retry broadcasting limit has been reached for the given event. + + Args: + event_id (Text): The ID of the broadcast event to validate + + Raises: + AppException: If the retry broadcasting limit has been reached + """ bot_settings = MongoProcessor.get_bot_settings(self.bot, self.user)
74-75: Enhance error logging with structured details.While the error handling is correct, consider enhancing the error logging to include more context about the broadcast event.
- logger.exception(e) + logger.exception(f"Broadcast failed for event_id={event_id}, is_resend={is_resend}: {str(e)}") exception = str(e) status = EVENT_STATUS.FAIL.valuekairon/shared/chat/broadcast/processor.py (2)
127-127: LGTM! Consider adding docstring for kwargs.The addition of **kwargs parameter makes the method more flexible and supports the broadcast template fix. Consider adding a docstring to document the expected kwargs fields for better maintainability.
Add a docstring like this:
def update_broadcast_logs_with_template(reference_id: Text, event_id: Text, raw_template: List[Dict], log_type: MessageBroadcastLogType, retry_count: int = 0, **kwargs): """ Update broadcast logs with template information and additional fields. Args: reference_id (Text): The reference ID for the broadcast event_id (Text): The event ID raw_template (List[Dict]): The template data log_type (MessageBroadcastLogType): The type of log retry_count (int, optional): Number of retries. Defaults to 0. **kwargs: Additional fields to update in the broadcast logs """
132-132: Consider adding field validation for kwargs.While the current implementation works correctly, consider adding validation for the kwargs fields to prevent unintended updates to the broadcast logs.
Example implementation:
ALLOWED_UPDATE_FIELDS = {'status', 'errors', 'api_response'} # add your allowed fields def update_broadcast_logs_with_template(reference_id: Text, event_id: Text, raw_template: List[Dict], log_type: MessageBroadcastLogType, retry_count: int = 0, **kwargs): invalid_fields = set(kwargs.keys()) - ALLOWED_UPDATE_FIELDS if invalid_fields: raise ValueError(f"Invalid update fields: {invalid_fields}") message_broadcast_logs = MessageBroadcastLogs.objects(reference_id=reference_id, event_id=event_id, log_type=log_type, retry_count=retry_count) message_broadcast_logs.update(template=raw_template, **kwargs)tests/unit_test/events/events_test.py (3)
2169-2170: Combine@patchdecorator onto a single line for clarityThe
@patchdecorator forsend_template_messageis split across two lines. Combining it into a single line improves readability.Apply this diff:
-@patch("kairon.chat.handlers.channels.clients.whatsapp.dialog360.BSP360Dialog.send_template_message", - autospec=True) +@patch("kairon.chat.handlers.channels.clients.whatsapp.dialog360.BSP360Dialog.send_template_message", autospec=True)
2231-2231: Use a more specific exception instead of genericExceptionUsing a generic
Exceptioninmock_list_templates.side_effectcan obscure the nature of the error and make debugging more difficult. Consider using a more specific exception class or defining a custom exception.Apply this diff:
-mock_list_templates.side_effect = Exception(template_exception) +class TemplateLoadException(Exception): + pass + +mock_list_templates.side_effect = TemplateLoadException(template_exception)
2211-2228: Refactortemplatedefinition to avoid duplicationThe
templatevariable is defined multiple times across different test methods. To improve maintainability, consider defining thetemplateonce at the class level or using a fixture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
kairon/events/definitions/message_broadcast.py(2 hunks)kairon/shared/channels/broadcast/whatsapp.py(4 hunks)kairon/shared/chat/broadcast/processor.py(1 hunks)tests/unit_test/events/events_test.py(2 hunks)
🔇 Additional comments (4)
kairon/events/definitions/message_broadcast.py (2)
58-58: LGTM: Improved status initialization.
Setting initial status to EVENT_STATUS.INITIATED.value provides better tracking of the event lifecycle.
Line range hint 63-71: Verify template handling in broadcast implementations.
The error handling looks good, but let's verify that all template-related edge cases are covered in the broadcast implementations.
✅ Verification successful
Let me gather more specific information about the template handling implementation.
Template handling implementation is complete and robust
The implementation shows comprehensive template handling with proper error management:
- Template retrieval and validation in
__get_templatewith exception handling - Detailed logging of template errors and execution status
- Proper handling of template parameters and language codes
- Robust retry mechanism with template state preservation
- Complete error tracking through
template_exceptionpropagation
The changes align well with the PR objectives of fixing broadcast template issues, with all edge cases covered in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for template handling in broadcast implementations
echo "Checking template handling in broadcast implementations..."
rg -l "template" --type py | grep -i "broadcast"
# Search for template-related error handling
echo "Checking template error handling..."
ast-grep --pattern 'try {
$$$
template
$$$
} catch {
$$$
}'
Length of output: 557
Script:
#!/bin/bash
# Check template handling in WhatsApp broadcast
echo "Checking WhatsApp broadcast template handling..."
rg -A 10 "template" kairon/shared/channels/broadcast/whatsapp.py
# Check template handling in processor
echo "Checking broadcast processor template handling..."
rg -A 10 "template" kairon/shared/chat/broadcast/processor.py
# Check template-related error handling
echo "Checking template error handling in broadcast..."
rg -B 2 -A 5 "except.*template" --type py | grep -i broadcast
Length of output: 13506
kairon/shared/channels/broadcast/whatsapp.py (1)
92-97: Properly handle template_exception after template retrieval
In the __send_using_pyscript method, while you are capturing template_exception, there is no check or handling based on its value. If an exception occurred during template retrieval, it might affect subsequent operations.
Consider verifying whether template_exception needs to be checked and handled appropriately after retrieval to prevent potential issues downstream. Here's a script to check where template_exception is utilized:
tests/unit_test/events/events_test.py (1)
2252-2254: Verify mocked bot settings reflect expected configurations
The mocked get_bot_settings returns hardcoded values. Ensure these configurations match the scenarios being tested and consider parameterizing them for different test cases.
| template_name = required_logs[0]["template_name"] | ||
| language_code = required_logs[0]["language_code"] | ||
| template, template_exception = self.__get_template(template_name, language_code) |
There was a problem hiding this comment.
Check if required_logs is not empty before accessing
The code assumes that required_logs has at least one element. If it's empty, accessing required_logs[0] will raise an IndexError.
Apply this diff to add a check:
+if not required_logs:
+ logger.warning("No required logs found for resending broadcast.")
+ return
template_name = required_logs[0]["template_name"]
language_code = required_logs[0]["language_code"]
template, template_exception = self.__get_template(template_name, language_code)📝 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.
| template_name = required_logs[0]["template_name"] | |
| language_code = required_logs[0]["language_code"] | |
| template, template_exception = self.__get_template(template_name, language_code) | |
| if not required_logs: | |
| logger.warning("No required logs found for resending broadcast.") | |
| return | |
| template_name = required_logs[0]["template_name"] | |
| language_code = required_logs[0]["language_code"] | |
| template, template_exception = self.__get_template(template_name, language_code) |
|
|
||
| for log in required_logs: | ||
| template_id = log["template_name"] | ||
| namespace = log["namespace"] | ||
| language_code = log["language_code"] | ||
| components = log["template_params"] | ||
| recipient = log["recipient"] | ||
| template = log["template"] | ||
| template = log.template if log.template else template |
There was a problem hiding this comment.
Correct dictionary access for template in log
The code uses log.template, but log is a dictionary. This should be changed to log["template"] to prevent an AttributeError.
Apply this diff to fix the issue:
-template = log.template if log.template else template
+template = log["template"] if log.get("template") else template📝 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.
| template = log.template if log.template else template | |
| template = log["template"] if log.get("template") else template |
| @@ -105,7 +107,7 @@ | |||
| namespace = template_config.get("namespace") | |||
| lang = template_config["language"] | |||
| template_params = self._get_template_parameters(template_config) | |||
| raw_template = self.__get_template(template_id, lang) | |||
| raw_template, template_exception = self.__get_template(template_id, lang) | |||
There was a problem hiding this comment.
Ensure template_exception is handled after calling __get_template
In the __send_using_configuration method, template_exception is retrieved but not checked or acted upon. Unhandled exceptions might lead to unexpected behavior.
Apply this diff to handle template_exception:
raw_template, template_exception = self.__get_template(template_id, lang)
+if template_exception:
+ # Handle the exception, for example, log it or raise an error
+ logger.error(f"Template exception occurred: {template_exception}")
+ raise AppException(f"Failed to retrieve template: {template_exception}")Committable suggestion skipped: line range outside the PR's diff.
| template_exception = None | ||
| template = [] | ||
| try: | ||
| for template in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}): | ||
| if template.get("language") == language: | ||
| template = template.get("components") | ||
| break | ||
| return template, template_exception | ||
| except Exception as e: | ||
| logger.exception(e) | ||
| template_exception = str(e) | ||
| return template, template_exception | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid variable shadowing of template in __get_template method
Using template as both the list to return and the loop variable can cause confusion and potential errors. Consider renaming the loop variable.
Apply this diff to rename the loop variable:
def __get_template(self, name: Text, language: Text):
template_exception = None
template_components = []
try:
- for template in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}):
- if template.get("language") == language:
- template = template.get("components")
+ for tmpl in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}):
+ if tmpl.get("language") == language:
+ template_components = tmpl.get("components")
break
- return template, template_exception
+ return template_components, template_exception
except Exception as e:
logger.exception(e)
template_exception = str(e)
- return template, template_exception
+ return template_components, template_exception📝 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.
| template_exception = None | |
| template = [] | |
| try: | |
| for template in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}): | |
| if template.get("language") == language: | |
| template = template.get("components") | |
| break | |
| return template, template_exception | |
| except Exception as e: | |
| logger.exception(e) | |
| template_exception = str(e) | |
| return template, template_exception | |
| template_exception = None | |
| template_components = [] | |
| try: | |
| for tmpl in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}): | |
| if tmpl.get("language") == language: | |
| template_components = tmpl.get("components") | |
| break | |
| return template_components, template_exception | |
| except Exception as e: | |
| logger.exception(e) | |
| template_exception = str(e) | |
| return template_components, template_exception |
| assert logged_config.pop("template_config") == [] | ||
| assert logged_config == config | ||
|
|
||
| timestamp = datetime.utcnow() |
There was a problem hiding this comment.
Avoid using pop in assertions to check dictionary contents
Using pop in assertions can lead to KeyError if the key is missing. It's safer to use get or check for the existence of the key before popping.
Consider refactoring as:
-logged_config.pop("_id")
-logged_config.pop("status")
-logged_config.pop("timestamp")
-assert logged_config.pop("template_config") == []
+assert logged_config.get("template_config") == []
+logged_config.pop("_id", None)
+logged_config.pop("status", None)
+logged_config.pop("timestamp", None)
+logged_config.pop("template_config", None)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/unit_test/events/events_test.py (4)
Line range hint
1287-1410: Consider adding assertions for template parametersThe test verifies broadcast logs but could be enhanced by adding explicit assertions for template parameters and their structure.
assert logs[0][0]['template_params'] == [{ 'type': 'header', 'parameters': [{ 'type': 'document', 'document': { 'link': 'https://drive.google.com/uc?export=download&id=1GXQ43jilSDelRvy1kr3PNNpl1e21dRXm', 'filename': 'Brochure.pdf' } }] }]
Line range hint
2846-4002: Comprehensive test for resend broadcast functionalityGood coverage of:
- Multiple retry scenarios
- Error handling
- Status tracking
- Campaign metrics validation
However, consider extracting common test data setup into helper methods to reduce code duplication.
def setup_broadcast_test_data(self, bot, user, reference_id, template, timestamp): """Helper to setup common test data""" MessageBroadcastLogs( reference_id=reference_id, log_type="common", bot=bot, status="Completed", user=user, timestamp=timestamp, # ... other common fields ).save() # ... setup other required test data
Line range hint
4002-4554: Verify error code handling more explicitlyThe test verifies meta error code handling but could be enhanced by:
- Adding assertions for specific error codes
- Testing boundary conditions
def test_meta_error_code_boundary_conditions(self): """Test boundary conditions for meta error codes""" # Test minimum error code assert not self.should_retry_broadcast({'code': 130000}) # Test maximum error code assert not self.should_retry_broadcast({'code': 139999})
5113-5113: Consider adding timeout tests for retry logicThe test covers multiple retries but should also verify timeout handling between retries.
@patch('time.sleep') # Mock sleep to speed up tests def test_retry_with_timeout(self, mock_sleep): """Test retry timeout handling""" # Setup test # Verify sleep is called between retries mock_sleep.assert_called_with(RETRY_TIMEOUT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/unit_test/events/events_test.py(12 hunks)
🔇 Additional comments (1)
tests/unit_test/events/events_test.py (1)
2165-2426: Well-structured test for template exception handling
The test case thoroughly validates the error handling when template loading fails. Good coverage of:
- Mock setup for template loading failure
- Verification of error propagation
- Validation of error logging
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
kairon/shared/channels/whatsapp/bsp/dialog360.py(1 hunks)
🧰 Additional context used
🪛 Ruff
kairon/shared/channels/whatsapp/bsp/dialog360.py
167-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
kairon/shared/channels/whatsapp/bsp/dialog360.py (1)
165-167: LGTM: Generic exception handling matches class pattern.
The addition of generic exception handling is consistent with other methods in the class and improves error visibility for template-related operations.
Let's verify the consistency of exception handling across template-related methods:
✅ Verification successful
Exception handling pattern is consistent across template operations
The verification confirms that the generic exception handling pattern is consistent across all template-related operations in the Dialog360 class:
- All methods have a similar try-catch structure
- They consistently catch both
DoesNotExistand genericException - All exceptions are logged using
logger.exception(e) - All caught exceptions are wrapped in
AppExceptionwith appropriate error messages - The specific code change follows this established pattern
The implementation maintains code consistency and proper error handling practices within the class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check exception handling patterns in template-related methods
# Expected: All template methods should have similar exception handling patterns
ast-grep --pattern 'def $method($$$) {
try {
$$$
} catch (DoesNotExist as e) {
$$$
} catch (Exception as e) {
$$$
}
}'
Length of output: 140
Script:
#!/bin/bash
# Let's try a simpler pattern to check exception handling in the file
rg "except Exception as e:" -B 5 -A 3 "kairon/shared/channels/whatsapp/bsp/dialog360.py"
# Also check for other exception handling patterns
rg "except \w+ as \w+:" -B 5 -A 3 "kairon/shared/channels/whatsapp/bsp/dialog360.py"
Length of output: 4055
🧰 Tools
🪛 Ruff
167-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| except Exception as e: | ||
| logger.exception(e) | ||
| raise AppException(str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception chaining to preserve the original traceback.
The exception handling looks good, but we should preserve the original exception's traceback using exception chaining.
Apply this diff to chain the exceptions:
except Exception as e:
logger.exception(e)
- raise AppException(str(e))
+ raise AppException(str(e)) from eThis change will help with debugging by preserving the full traceback of the original exception.
📝 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.
| except Exception as e: | |
| logger.exception(e) | |
| raise AppException(str(e)) | |
| except Exception as e: | |
| logger.exception(e) | |
| raise AppException(str(e)) from e |
🧰 Tools
🪛 Ruff
167-167: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
Broadcast template issue handled and added test cases related to the same.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests