mail channel dispatch optional #1953
Conversation
WalkthroughA new configuration parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot
participant EmailAction
participant SMTPServer
User->>Bot: Triggers email action
Bot->>EmailAction: Execute with config (dispatch_bot_response)
EmailAction->>SMTPServer: Send email
SMTPServer-->>EmailAction: Acknowledge
alt dispatch_bot_response == True
EmailAction->>Bot: Dispatch bot response message
else dispatch_bot_response == False
EmailAction-->>Bot: Skip dispatching response
end
Bot-->>User: (Conditional) Bot response
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 0
🧹 Nitpick comments (9)
tests/integration_test/services_test.py (3)
23145-23145: Maintain consistent quoting style
Currently the new key uses single quotes, whereas the surrounding dict literals use double quotes for keys. Please update to:- 'dispatch_bot_response': True, + "dispatch_bot_response": True,
23165-23165: Maintain consistent quoting style
The key here also uses single quotes; switch to double quotes to match adjacent entries:- 'dispatch_bot_response': True, + "dispatch_bot_response": True,
23185-23185: Maintain consistent quoting style
Please change the single-quoted key to double quotes for consistency:- 'dispatch_bot_response': True, + "dispatch_bot_response": True,kairon/shared/data/processor.py (1)
7152-7152: Consider using a default value for dispatch_bot_responseFor consistency with other action types, consider using the
.get()method with a default value fordispatch_bot_response, similar to how it's handled in other action types likeprompt_action(line 7936) andschedule_action(line 8736).- email_action.dispatch_bot_response = action["dispatch_bot_response"] + email_action.dispatch_bot_response = action.get("dispatch_bot_response", True)This would prevent KeyError exceptions if the parameter is missing in the request data and maintain consistent behavior across different action types.
tests/unit_test/data_processor/data_processor_test.py (2)
16337-16354: Good test coverage for the dispatch_bot_response=False scenario.This test properly verifies that an email action can be created with the
dispatch_bot_responseflag set toFalse. The cleanup at the end is a good practice.However, there's an unused variable in the test:
- with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: + with patch("kairon.shared.utils.SMTP", autospec=True):🧰 Tools
🪛 Ruff (0.8.2)
16351-16351: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16334-16334: Fix unused variables in tests.There are several instances of unused
mock_smtpvariables throughout the test file. While this doesn't affect functionality, it triggers linter warnings.Consider updating these instances to:
- with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: + with patch("kairon.shared.utils.SMTP", autospec=True):Or if you need to verify the mock was called:
with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: # Test code mock_smtp.assert_called_once() # Or appropriate assertionAlso applies to: 16351-16351, 16410-16410, 16472-16472, 16491-16491, 16509-16509, 16514-16514, 16531-16531
🧰 Tools
🪛 Ruff (0.8.2)
16334-16334: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
tests/integration_test/action_service_test.py (3)
6150-6150: Fix typo in function nameThere's a typo in the function name: "dispaatch" has an extra "a". This should be corrected for clarity and consistency.
-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):
6151-6158: Use context managers for file operationsFile operations should use context managers (
withstatements) to ensure proper resource cleanup.- 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)
6151-6151: Use a context manager for opening files
(SIM115)
6153-6153: Use a context manager for opening files
(SIM115)
6155-6155: Use a context manager for opening files
(SIM115)
6157-6157: Use a context manager for opening files
(SIM115)
6275-6275: Consider using conventional comparison orderWhile Yoda conditions (
{} == kwargs) are valid, the standard Python style is to place the variable before the constant.- assert {} == kwargs + assert kwargs == {}Also applies to: 6282-6282, 6290-6290
🧰 Tools
🪛 Ruff (0.8.2)
6275-6275: 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
🪛 Ruff (0.8.2)
tests/integration_test/action_service_test.py
6151-6151: Use a context manager for opening files
(SIM115)
6153-6153: Use a context manager for opening files
(SIM115)
6155-6155: Use a context manager for opening files
(SIM115)
6157-6157: Use a context manager for opening files
(SIM115)
6275-6275: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
6282-6282: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
6290-6290: Yoda condition detected
Rewrite as kwargs == {}
(SIM300)
tests/unit_test/data_processor/data_processor_test.py
16334-16334: Local variable mock_smtp is assigned to but never used
Remove assignment to unused variable mock_smtp
(F841)
16351-16351: 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 (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
🔇 Additional comments (15)
kairon/shared/data/data_models.py (1)
1004-1004: Looks good! New parameter adds flexibility for email actions.The addition of
dispatch_bot_responseparameter with a default value ofTrueprovides a good way to control whether the bot should send a response message to the user after executing an email action. The default value ensures backward compatibility with existing behavior.kairon/shared/actions/data_objects.py (1)
447-447: LGTM! Field added consistently with request model.The addition of the
dispatch_bot_responsefield to theEmailActionConfigclass matches the corresponding change in the request model. Setting the default toTruemaintains backward compatibility with existing database records.kairon/actions/definitions/email.py (2)
57-57: Good addition of the dispatch control flag with a default value.This change adds a new configuration parameter
dispatch_bot_responsewith a sensible default value ofTrue, ensuring backward compatibility with existing configurations. The naming clearly indicates the purpose of this flag.
91-92: Good implementation of conditional response dispatch.The modification makes the bot response dispatch conditional based on the
dispatch_bot_responseflag, allowing flexibility in whether responses are sent to the user after email actions. The placement inside thefinallyblock ensures this logic runs regardless of whether the email action succeeds or fails.tests/unit_test/data_processor/data_processor_test.py (8)
16331-16331: Properly added dispatch_bot_response parameter.This addition correctly implements the new
dispatch_bot_responseparameter with a default value ofTrue, which aligns with the PR objective to make mail channel dispatch optional.
16366-16367: Proper configuration of dispatch_bot_response parameter.The addition of the
dispatch_bot_responseparameter set toTrueis consistent with the PR objectives and other test updates.
16406-16407: Consistently added dispatch_bot_response parameter.The parameter has been consistently added across test cases, which ensures thorough test coverage for the new feature.
16468-16469: Properly added dispatch_bot_response parameter.The addition maintains consistency with other test cases.
16487-16488: Properly added dispatch_bot_response parameter.The parameter has been properly added with a value of
True.
16506-16506: Good test coverage for the False case in edit action.Testing the edit functionality with
dispatch_bot_responseset toFalseensures comprehensive coverage of the new feature.
16513-16513: Properly updating dispatch_bot_response in the edit test.This correctly tests changing the parameter value during an edit operation, which is an important test case.
16528-16528: Consistently implemented dispatch_bot_response parameter.All the relevant test cases have been updated to include the new parameter.
tests/integration_test/action_service_test.py (3)
6161-6174: LGTM! Good implementation of the newdispatch_bot_responseparameterThe test properly configures the email action with
dispatch_bot_response=False, which is the key feature being introduced in this PR.
6264-6266: Good validation of the dispatch behaviorThis section correctly verifies that when
dispatch_bot_response=False:
- The action successfully completes (status code 200)
- The slot is updated with the expected value
- No responses are sent to the user (
len(response_json['responses']) == 0)This effectively validates the core functionality introduced in this PR.
6147-6297: Great test coverage for the new featureThis comprehensive test properly validates that:
- Email actions can be configured with
dispatch_bot_response=False- The email is still sent correctly via SMTP with all expected parameters
- The bot doesn't dispatch a response to the user when this flag is disabled
- The action logic still updates the slot with the response message
This provides good coverage for the new conditional dispatch feature.
🧰 Tools
🪛 Ruff (0.8.2)
6151-6151: Use a context manager for opening files
(SIM115)
6153-6153: Use a context manager for opening files
(SIM115)
6155-6155: Use a context manager for opening files
(SIM115)
6157-6157: Use a context manager for opening files
(SIM115)
6275-6275: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
6282-6282: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
6290-6290: Yoda condition detected
Rewrite as
kwargs == {}(SIM300)
2 participants
Summary by CodeRabbit
New Features
Tests