Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcast template fix #1592

Closed
wants to merge 4 commits into from
Closed

Broadcast template fix #1592

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
656a2e4
Broadcast template issue handled and added test cases related to the …
maheshsattala Nov 7, 2024
d8847a2
Broadcast template issue handled and added test cases related to the …
maheshsattala Nov 8, 2024
5481027
fixed failing test cases
maheshsattala Nov 8, 2024
e154860
Broadcast template issue handled and added test cases related to the …
maheshsattala Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion kairon/events/definitions/message_broadcast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def execute(self, event_id: Text, **kwargs):
"""
config = None
reference_id = None
status = EVENT_STATUS.FAIL.value
status = EVENT_STATUS.INITIATED.value
exception = None
is_resend = kwargs.get('is_resend', "False") == "True"
try:
Expand All @@ -72,6 +72,7 @@ def execute(self, event_id: Text, **kwargs):
except Exception as e:
logger.exception(e)
exception = str(e)
status = EVENT_STATUS.FAIL.value
finally:
time.sleep(5)
MessageBroadcastProcessor.insert_status_received_on_channel_webhook(reference_id, config["name"],
Expand Down
33 changes: 24 additions & 9 deletions kairon/shared/channels/broadcast/whatsapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,12 @@ def log(**kwargs):
template_name = self.config['template_name']
language_code = self.config['language_code']

raw_template = self.__get_template(template_name, language_code)
raw_template, template_exception = self.__get_template(template_name, language_code)
raw_template = raw_template if raw_template else []
MessageBroadcastProcessor.update_broadcast_logs_with_template(
self.reference_id, self.event_id, raw_template=raw_template,
log_type=MessageBroadcastLogType.send.value, retry_count=0
log_type=MessageBroadcastLogType.send.value, retry_count=0,
template_exception=template_exception
)

def __send_using_configuration(self, recipients: List):
Expand All @@ -105,7 +107,7 @@ def __send_using_configuration(self, recipients: List):
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


# if there's no template body, pass params as None for all recipients
template_params = template_params * len(recipients) if template_params else [template_params] * len(recipients)
Expand All @@ -128,7 +130,7 @@ def __send_using_configuration(self, recipients: List):
self.bot, MessageBroadcastLogType.send.value, self.reference_id, api_response=response,
status=status, recipient=recipient, template_params=t_params, template=raw_template,
event_id=self.event_id, template_name=template_id, language_code=lang, namespace=namespace,
retry_count=0
retry_count=0, template_exception=template_exception
)
MessageBroadcastProcessor.add_event_log(
self.bot, MessageBroadcastLogType.common.value, self.reference_id, failure_cnt=failure_cnt, total=total,
Expand All @@ -151,14 +153,17 @@ def resend_broadcast(self):
failure_cnt = 0
total = len(required_logs)
skipped_count = len(broadcast_logs) - total
template_name = required_logs[0]["template_name"]
language_code = required_logs[0]["language_code"]
template, template_exception = self.__get_template(template_name, language_code)
Comment on lines +156 to +158
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
template = log.template if log.template else template
template = log["template"] if log.get("template") else template

response = channel_client.send_template_message(template_id, recipient, language_code, components,
namespace)
status = "Failed" if response.get("error") else "Success"
Expand All @@ -169,7 +174,7 @@ def resend_broadcast(self):
self.bot, MessageBroadcastLogType.resend.value, self.reference_id, api_response=response,
status=status, recipient=recipient, template_params=components, template=template,
event_id=self.event_id, template_name=template_id, language_code=language_code, namespace=namespace,
retry_count=retry_count,
retry_count=retry_count, template_exception=template_exception
)
kwargs = {
f"resend_count_{retry_count}": total,
Expand Down Expand Up @@ -197,6 +202,16 @@ def __get_client(self):
raise AppException(f"Whatsapp channel config not found!")

def __get_template(self, name: Text, language: Text):
for template in BSP360Dialog(self.bot, self.user).list_templates(**{"business_templates.name": name}):
if template.get("language") == language:
return template.get("components")
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

Comment on lines +205 to +217
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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

4 changes: 2 additions & 2 deletions kairon/shared/chat/broadcast/processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,12 @@ def get_reference_id_from_broadcasting_logs(event_id):
@staticmethod
def update_broadcast_logs_with_template(reference_id: Text, event_id: Text, raw_template: List[Dict],
log_type: MessageBroadcastLogType,
retry_count: int = 0):
retry_count: int = 0, **kwargs):
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)
message_broadcast_logs.update(template=raw_template, **kwargs)

@staticmethod
def extract_message_ids_from_broadcast_logs(reference_id: Text, retry_count: int = 0):
Expand Down
Loading
Loading