mail channel dispatch optional #1943
Conversation
WalkthroughA new configuration parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot
participant EmailAction
participant Dispatcher
User->>Bot: Triggers email action
Bot->>EmailAction: Execute email action
EmailAction->>EmailAction: Send email (SMTP)
alt dispatch_bot_response == True
EmailAction->>Dispatcher: utter_message(bot_response)
else dispatch_bot_response == False
EmailAction-->>Dispatcher: (No message dispatched)
end
EmailAction-->>Bot: Return result
Bot-->>User: (Depends on dispatch_bot_response)
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (7)
tests/integration_test/services_test.py (3)
23340-23340: Ensure consistency in key quoting
The addition of'dispatch_bot_response': Truecorrectly aligns the test with the new default flag behavior. For readability and consistency with the surrounding JSON‐style keys, consider using double quotes ("dispatch_bot_response") instead of single quotes.
23360-23360: Ensure consistency in key quoting
The addition of'dispatch_bot_response': Truecorrectly aligns the test with the new default flag behavior. For readability and consistency with the surrounding JSON‐style keys, consider using double quotes ("dispatch_bot_response") instead of single quotes.
23380-23380: Ensure consistency in key quoting
The addition of'dispatch_bot_response': Truecorrectly aligns the test with the new default flag behavior. For readability and consistency with the surrounding JSON‐style keys, consider using double quotes ("dispatch_bot_response") instead of single quotes.kairon/actions/definitions/email.py (1)
44-52: Consider updating docstring to reflect new behavior.The docstring could be updated to mention the new
dispatch_bot_responseparameter and its purpose, explaining that the bot response dispatch is now optional based on configuration.@param dispatcher: Client to send messages back to the user. @param tracker: Tracker object to retrieve slots, events, messages and other contextual information. @param domain: Bot domain +Note: Bot response dispatch to the user can be controlled via the `dispatch_bot_response` configuration parameter. :return: Dict containing slot name as keys and their values.tests/integration_test/action_service_test.py (3)
6156-6156: Fix typo in function nameThere's a typo in the function name: "dispaatch" should be "dispatch".
-def test_email_action_execution_dispaatch_false(mock_smtp, mock_action_config, mock_action): +def test_email_action_execution_dispatch_false(mock_smtp, mock_action_config, mock_action):
6281-6281: Avoid Yoda conditionsThe code uses Yoda conditions (
{} == kwargs) which are less readable. It's better to put the variable first in the comparison.- assert {} == kwargs + assert kwargs == {}Also applies to: 6288-6288, 6296-6296
🧰 Tools
🪛 Ruff (0.8.2)
6281-6281: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
6153-6304: Consider extracting duplicate SMTP assertion codeThis test contains nearly identical SMTP assertion code as other email action tests. Consider extracting these assertions into a helper method to reduce code duplication.
You could create a helper method like:
def assert_smtp_calls(mock_smtp, action_config, expected_subject=None): """Assert that SMTP calls were made correctly with the given config.""" # Assert connection name, args, kwargs = mock_smtp.method_calls.pop(0) assert name == '().connect' assert kwargs == {} host, port = args assert host == action_config.smtp_url assert port == action_config.smtp_port # Assert login name, args, kwargs = mock_smtp.method_calls.pop(0) assert name == '().login' assert kwargs == {} from_email, password = args assert from_email == action_config.from_email.value assert password == action_config.smtp_password.value # Assert sendmail name, args, kwargs = mock_smtp.method_calls.pop(0) assert name == '().sendmail' assert kwargs == {} assert args[0] == action_config.from_email.value assert args[1] == action_config.to_email.value assert str(args[2]).__contains__(action_config.subject) assert str(args[2]).__contains__("Content-Type: text/html") if expected_subject: assert str(args[2]).__contains__(f"Subject: {expected_subject}")Then use it in your tests:
assert_smtp_calls(mock_smtp, action_config, expected_subject="default test")🧰 Tools
🪛 Ruff (0.8.2)
6157-6157: Use a context manager for opening files
(SIM115)
6159-6159: Use a context manager for opening files
(SIM115)
6161-6161: Use a context manager for opening files
(SIM115)
6163-6163: Use a context manager for opening files
(SIM115)
6281-6281: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
6288-6288: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
6296-6296: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kairon/actions/definitions/email.py(2 hunks)kairon/shared/actions/data_objects.py(1 hunks)kairon/shared/data/data_models.py(1 hunks)kairon/shared/data/processor.py(1 hunks)tests/integration_test/action_service_test.py(1 hunks)tests/integration_test/services_test.py(3 hunks)tests/unit_test/data_processor/data_processor_test.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit_test/data_processor/data_processor_test.py (4)
tests/unit_test/api/api_processor_test.py (1)
mock_smtp(1189-1190)tests/integration_test/services_test.py (1)
mock_smtp(115-116)kairon/shared/data/processor.py (2)
add_email_action(7085-7107)edit_email_action(7109-7156)kairon/shared/actions/data_objects.py (1)
EmailActionConfig(437-493)
🪛 Ruff (0.8.2)
tests/integration_test/action_service_test.py
6157-6157: Use a context manager for opening files
(SIM115)
6159-6159: Use a context manager for opening files
(SIM115)
6161-6161: Use a context manager for opening files
(SIM115)
6163-6163: Use a context manager for opening files
(SIM115)
6281-6281: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
6288-6288: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
6296-6296: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
tests/unit_test/data_processor/data_processor_test.py
16056-16056: Local variable mock_smtp is assigned to but never used
Remove assignment to unused variable mock_smtp
(F841)
16073-16073: Local variable mock_smtp is assigned to but never used
Remove assignment to unused variable mock_smtp
(F841)
⏰ 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 (15)
kairon/shared/actions/data_objects.py (1)
447-447: Clean implementation of the new configuration parameter.The addition of the
dispatch_bot_responseboolean field with a default value ofTruealigns perfectly with the PR objective of making bot response dispatch optional for email actions. The default value ensures backward compatibility with existing behavior.kairon/shared/data/data_models.py (1)
1004-1004: Good consistency in implementation across models.The
dispatch_bot_responsefield addition in theEmailActionRequestPydantic model mirrors the same field added to theEmailActionConfigclass, ensuring consistency across the data models and request handling. The default value ofTruemaintains backward compatibility.kairon/shared/data/processor.py (1)
7152-7152: Support added for controlling bot response dispatching in email actions.This change assigns the
dispatch_bot_responseflag from the input parameters to the email action configuration, allowing users to control whether bot responses are sent to users after email actions are executed.kairon/actions/definitions/email.py (2)
57-57: Clean implementation of new configuration parameter.The addition of
dispatch_bot_responsewith a default value ofTrueensures backward compatibility with existing configurations that don't have this parameter defined. The parameter name is descriptive and follows the naming conventions in the codebase.
91-92: Good implementation of conditional bot response dispatch.The conditional check correctly implements the feature described in the PR title, making the bot response dispatch optional based on configuration. The code is clearly structured with proper indentation.
tests/integration_test/action_service_test.py (2)
6166-6180: The action configuration looks goodThe test correctly sets up the
EmailActionConfigwithdispatch_bot_response=False, which is the key parameter being tested in this function.
6270-6275: Good assertions for testing non-dispatch behaviorThese assertions correctly verify that when
dispatch_bot_response=False:
- No bot responses are returned (empty responses list)
- The slot is still updated with the action response
This confirms the expected behavior that the action executes but doesn't dispatch a response to the user.
tests/unit_test/data_processor/data_processor_test.py (8)
16053-16053: Good addition of the new dispatch_bot_response parameter.Adding the
dispatch_bot_responseparameter with default valueTruealigns with the implementation in theEmailActionConfigmodel.
16059-16075: Excellent test coverage for the new feature!This test properly verifies adding an email action with
dispatch_bot_responseset toFalse. It appropriately tests the new functionality and includes proper cleanup by deleting the test action afterward.The mock_smtp variables in these tests are flagged by static analysis but are correctly used as part of the patch context manager pattern, which is a standard testing approach.
🧰 Tools
🪛 Ruff (0.8.2)
16073-16073: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16088-16089: Good parametrization.Adding the
dispatch_bot_responseparameter to this custom text test case ensures consistent test coverage.
16128-16129: Good inclusion of the parameter in additional test case.Consistently adding the parameter across all relevant test cases ensures thorough coverage.
16190-16191: Proper test coverage continued.Maintaining consistent test patterns with the new parameter across all email action tests.
16209-16210: Appropriate parameter addition for slot-based test.The
dispatch_bot_responseparameter is correctly added to this slot-based test scenario.
16228-16228: Well-structured test for both True and False values!This test effectively covers both values of the
dispatch_bot_responseparameter:
- Initially set to
Falsefor the first edit test- Then explicitly updated to
Truefor the second edit test with custom text slotThis approach provides good coverage of the editing functionality with the new parameter.
Also applies to: 16235-16235
16250-16250: Consistent parameter inclusion.The
dispatch_bot_responseparameter is consistently added throughout all relevant test cases, ensuring thorough test coverage.
| Utility.email_conf['email']['templates']['conversation'] = open('template/emails/conversation.html', | ||
| 'rb').read().decode() | ||
| Utility.email_conf['email']['templates']['bot_msg_conversation'] = open( | ||
| 'template/emails/bot_msg_conversation.html', 'rb').read().decode() | ||
| Utility.email_conf['email']['templates']['user_msg_conversation'] = open( | ||
| 'template/emails/user_msg_conversation.html', 'rb').read().decode() | ||
| Utility.email_conf['email']['templates']['button_template'] = open('template/emails/button.html', | ||
| 'rb').read().decode() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use context managers for file operations
Opening files without context managers can lead to resource leaks if exceptions occur. Use with statements to ensure files are properly closed.
- Utility.email_conf['email']['templates']['conversation'] = open('template/emails/conversation.html',
- 'rb').read().decode()
- Utility.email_conf['email']['templates']['bot_msg_conversation'] = open(
- 'template/emails/bot_msg_conversation.html', 'rb').read().decode()
- Utility.email_conf['email']['templates']['user_msg_conversation'] = open(
- 'template/emails/user_msg_conversation.html', 'rb').read().decode()
- Utility.email_conf['email']['templates']['button_template'] = open('template/emails/button.html',
- 'rb').read().decode()
+ with open('template/emails/conversation.html', 'rb') as f:
+ Utility.email_conf['email']['templates']['conversation'] = f.read().decode()
+ with open('template/emails/bot_msg_conversation.html', 'rb') as f:
+ Utility.email_conf['email']['templates']['bot_msg_conversation'] = f.read().decode()
+ with open('template/emails/user_msg_conversation.html', 'rb') as f:
+ Utility.email_conf['email']['templates']['user_msg_conversation'] = f.read().decode()
+ with open('template/emails/button.html', 'rb') as f:
+ Utility.email_conf['email']['templates']['button_template'] = f.read().decode()📝 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.
| Utility.email_conf['email']['templates']['conversation'] = open('template/emails/conversation.html', | |
| 'rb').read().decode() | |
| Utility.email_conf['email']['templates']['bot_msg_conversation'] = open( | |
| 'template/emails/bot_msg_conversation.html', 'rb').read().decode() | |
| Utility.email_conf['email']['templates']['user_msg_conversation'] = open( | |
| 'template/emails/user_msg_conversation.html', 'rb').read().decode() | |
| Utility.email_conf['email']['templates']['button_template'] = open('template/emails/button.html', | |
| 'rb').read().decode() | |
| with open('template/emails/conversation.html', 'rb') as f: | |
| Utility.email_conf['email']['templates']['conversation'] = f.read().decode() | |
| with open('template/emails/bot_msg_conversation.html', 'rb') as f: | |
| Utility.email_conf['email']['templates']['bot_msg_conversation'] = f.read().decode() | |
| with open('template/emails/user_msg_conversation.html', 'rb') as f: | |
| Utility.email_conf['email']['templates']['user_msg_conversation'] = f.read().decode() | |
| with open('template/emails/button.html', 'rb') as f: | |
| Utility.email_conf['email']['templates']['button_template'] = f.read().decode() |
🧰 Tools
🪛 Ruff (0.8.2)
6157-6157: Use a context manager for opening files
(SIM115)
6159-6159: Use a context manager for opening files
(SIM115)
6161-6161: Use a context manager for opening files
(SIM115)
6163-6163: Use a context manager for opening files
(SIM115)
1 participant
Summary by CodeRabbit
New Features
Tests