added support for custom body text for whatsapp converter button #1839
Conversation
WalkthroughThe changes introduce a conditional check within the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (4)
kairon/chat/converters/channels/whatsapp.py (4)
74-75: Handle potential missing text in label itemsThe added code correctly allows for custom body text for WhatsApp converter buttons. However, it doesn't handle the case where
item.get("text")might returnNone.I recommend adding a check to ensure text exists before using it:
if item.get("type") == 'label': - body_msg = {"text": item.get("text")} + label_text = item.get("text") + if label_text is not None: + body_msg = {"text": label_text}
74-75: Add documentation about the new label featureWhile the implementation is straightforward, there's no documentation or comments explaining this new feature, its purpose, or how to structure inputs to take advantage of it.
I recommend adding a comment above or within the method to explain how the label type works:
def button_transformer(self, message): try: message_template = ElementTransformerOps.getChannelConfig(self.channel, self.message_type) button_json_temp = json.loads(message_template) jsoniterator = ElementTransformerOps.json_generator(message) buttons = {"buttons": []} body_default = ElementTransformerOps.getChannelConfig(self.channel, "body_message") body_msg = {"text": body_default} + # Process message items, including custom labels for body text and button elements for item in jsoniterator:
74-83: Consider refactoring repeated conditional logicThe implementation pattern for 'label' and 'button' types follows a similar structure - checking for a specific type and then updating a related variable. As more item types are supported, this pattern might become unwieldy.
For better maintainability, consider using a strategy pattern or a dispatch dictionary:
def button_transformer(self, message): try: message_template = ElementTransformerOps.getChannelConfig(self.channel, self.message_type) button_json_temp = json.loads(message_template) jsoniterator = ElementTransformerOps.json_generator(message) buttons = {"buttons": []} body_default = ElementTransformerOps.getChannelConfig(self.channel, "body_message") body_msg = {"text": body_default} + # Process each item based on its type for item in jsoniterator: - if item.get("type") == 'label': - body_msg = {"text": item.get("text")} - if item.get("type") == ElementTypes.BUTTON.value: - title = ElementTransformerOps.json_generator(item.get("children")) - for titletext in title: - button_text = titletext.get("text") - btn_body = {} - btn_body.update({"type": "reply"}) - btn_body.update({"reply": {"id": item.get("value"), "title": button_text}}) - buttons["buttons"].append(btn_body) + item_type = item.get("type") + if item_type == 'label': + label_text = item.get("text") + if label_text is not None: + body_msg = {"text": label_text} + elif item_type == ElementTypes.BUTTON.value: + title = ElementTransformerOps.json_generator(item.get("children")) + button_text = next((titletext.get("text") for titletext in title), None) + if button_text: + btn_body = { + "type": "reply", + "reply": {"id": item.get("value"), "title": button_text} + } + buttons["buttons"].append(btn_body)
74-75: Consider implementing unit tests for the new featureThe PR adds new functionality (customizable button body text) but doesn't appear to include tests.
I recommend adding unit tests that verify:
- Default body message is used when no label is present
- Custom label text is used when available
- The last label is used if multiple labels are present
Would you like me to help draft these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kairon/chat/converters/channels/whatsapp.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
kairon/chat/converters/channels/whatsapp.py (1)
74-75:❓ Verification inconclusive
Consider handling multiple label items
Currently, if multiple items have type 'label', only the last one's text will be used as the body message since each occurrence will overwrite the previous
body_msgvalue.Is this the intended behavior? If not, you might want to:
- Use only the first label found
- Combine multiple labels
- Add documentation specifying that only the last label is used
🏁 Script executed:
#!/bin/bash # Check if there are any usage examples in the codebase that might help understand if # multiple labels in one message is a valid scenario echo "Searching for examples of WhatsApp button message construction..." rg -A 3 -B 3 "type.*label" --glob "*.py" --glob "*.json" echo "Searching for test cases related to WhatsappResponseConverter button_transformer..." rg -A 3 -B 3 "WhatsappResponseConverter.*button_transformer" --glob "*test*.py"Length of output: 1205
Multiple label handling in WhatsApp conversion logic
The code currently updatesbody_msgeach time an item with type'label'is encountered, meaning if there are multiple such items in the message, only the last one will be kept. Our search did not reveal any test cases or usage examples that clarify whether multiple labels should be combined or if only one label is expected. Please verify the intended design. Consider one of the following actions:
If only one label is expected:
- Document the behavior clearly to indicate that only the last label is used, or
- Update the logic to pick the first label and ignore subsequent ones if that meets requirements.
If multiple labels might occur:
- Implement logic to combine them, or
- Add test cases illustrating the desired behavior.
File:
kairon/chat/converters/channels/whatsapp.py(Lines 74-75)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
kairon/shared/llm/processor.py (1)
133-133: Consider potential API compatibility concerns.You've added a new 'context' key to the response dictionary in the success case, which exposes the context data to consumers of this API. This change has two potential issues:
- API consumers might not be expecting this additional field, which could cause compatibility issues.
- The error response on line 141 doesn't include a 'context' key, creating an inconsistent response structure between success and error cases.
I'd recommend making one of these changes:
- response = {"content": answer, "context": context} + response = {"content": answer, "context": context}And update the error response for consistency:
- response = {"is_failure": True, "exception": str(e), "content": None} + response = {"is_failure": True, "exception": str(e), "content": None, "context": None}Or document this API change clearly if it's an intentional feature to support the custom body text for WhatsApp converter buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kairon/shared/llm/processor.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
🔇 Additional comments (1)
kairon/shared/llm/processor.py (1)
119-145:❓ Verification inconclusive
Verify the impact on API consumers.
The addition of the 'context' field to the response structure modifies the API contract of the
predictmethod. While this change supports the WhatsApp converter button functionality, it's important to ensure all consumers of this API are prepared for this change.
🏁 Script executed:
#!/bin/bash # Find all places where the predict method is called to verify they can handle the new response structure echo "Searching for calls to the predict method..." rg -A 5 "predict\s*\(" --type py | grep -v "def predict" echo "Searching for places where the response from predict is processed..." rg -A 5 "response.*predict" --type pyLength of output: 23692
Action: Confirm API consumer compatibility with the new "context" field in predict responses.
After verifying the call sites and tests, we can see that the majority of consumers (including those in unit tests, prompt handling, and message processing) retrieve the response from the
predictmethod by accessing keys like"content"or checking for failure states. These components appear to handle the returned dict without adverse effects from the additional"context"field. However, since this change alters the API contract, please ensure that any external or downstream consumers (including those not covered by our tests) are aware of and prepared for the new field.
- Files to review:
kairon/actions/definitions/prompt.py– Verify that only the necessary parts of the response are used.kairon/chat/agent/message_processor.py– Confirm that the response is processed in a way that accommodates extra fields.- Unit tests in
tests/unit_test/llm_test.pyalready validate the handling of the response structure.Keep a close eye on any external integrations that might depend on the prior response structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration_test/action_service_test.py (3)
13626-13627: Consider safer context removal and add explanatory comment.The code correctly removes the 'context' key from the llm_response_log, which aligns with the new structure returned by the LLMProcessor. However, consider:
- Using the safer
pop('context', None)pattern to prevent potential KeyError if the key doesn't exist- Adding a small comment explaining why context needs to be removed (likely for test assertion compatibility)
if event.get('llm_response_log'): - event['llm_response_log'].pop('context') + # Remove context to maintain compatibility with test assertions + event['llm_response_log'].pop('context', None)
13759-13760: Consider safer context removal and add explanatory comment.The code correctly removes the 'context' key from the llm_response_log, which aligns with the new structure returned by the LLMProcessor. However, consider:
- Using the safer
pop('context', None)pattern to prevent potential KeyError if the key doesn't exist- Adding a small comment explaining why context needs to be removed (likely for test assertion compatibility)
if event.get('llm_response_log'): - event['llm_response_log'].pop('context') + # Remove context to maintain compatibility with test assertions + event['llm_response_log'].pop('context', None)
13913-13914: Consider safer context removal and add explanatory comment.The code correctly removes the 'context' key from the llm_response_log, which aligns with the new structure returned by the LLMProcessor. However, consider:
- Using the safer
pop('context', None)pattern to prevent potential KeyError if the key doesn't exist- Adding a small comment explaining why context needs to be removed (likely for test assertion compatibility)
if event.get('llm_response_log'): - event['llm_response_log'].pop('context') + # Remove context to maintain compatibility with test assertions + event['llm_response_log'].pop('context', None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/action_service_test.py(3 hunks)tests/unit_test/llm_test.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unit_test/llm_test.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (2)
tests/integration_test/action_service_test.py (2)
13909-13909: Good validation of context presence.The assertion confirms that the new 'context' field is properly populated in the response, which is crucial for validating the feature's implementation.
13909-13909:✅ Verification successful
Verify all related tests handle the new context field consistently.
The changes ensure this test can handle the new 'context' field in the LLM response. The assertion confirms the field has content before removing it for comparison.
Also applies to: 13913-13914
🏁 Script executed:
#!/bin/bash # Check if other test files might need similar updates to handle the new context field echo "Searching for other test files that might need similar updates..." rg -l "llm_response_log" --type py --glob "tests/**" echo "Checking for other instances where context might need to be handled:" rg "llm_response_log" --type py --glob "tests/**" -A 5 -B 5Length of output: 6919
Consistent Handling of 'context' Field Verified
The search confirms that the only tests involving the
llm_response_log(and its newcontextfield) are intests/integration_test/action_service_test.py. In this file, the assertion at line 13909 correctly verifies thatcontextis non-empty before it is removed later (lines 13913–13914). No additional test files require similar updates.
Summary by CodeRabbit
New Features
Bug Fixes
llm_response_logstructure in test cases to ensure proper handling of the context.Refactor