Email dispatch flag fix #1931
Email dispatch flag fix #1931
Conversation
code_rabbit_change
code_rabbit_change
all test case success
WalkthroughThis change introduces a new configuration parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot
participant ActionEmail
participant Dispatcher
User->>Bot: Triggers email action
Bot->>ActionEmail: Execute email action
ActionEmail->>ActionEmail: Check config for dispatch_response
alt dispatch_response is True and bot_response is not empty
ActionEmail->>Dispatcher: utter_message(bot_response)
else dispatch_response is False or bot_response is empty
ActionEmail-->>Dispatcher: (No message dispatched)
end
ActionEmail-->>Bot: Return execution result
Bot-->>User: Responds (if any)
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:
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
🔭 Outside diff range comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
15965-16223:⚠️ Potential issueMissing tests for the dispatch_response parameter functionality
Throughout the test file changes, I don't see any test that specifically verifies the behavior of the new
dispatch_responseparameter for conditional dispatching of bot responses. This is a key feature mentioned in the PR summary.Consider adding a specific test case that verifies email actions behave correctly with both
TrueandFalsevalues for thedispatch_responseparameter. For example:def test_email_action_with_dispatch_response_parameter(self): processor = MongoProcessor() # Test with dispatch_response = True email_config_with_dispatch = { "action_name": "email_with_dispatch", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, "dispatch_response": True } # Test with dispatch_response = False email_config_without_dispatch = { "action_name": "email_without_dispatch", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, "dispatch_response": False } with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: # Add both configurations action_id1 = processor.add_email_action(email_config_with_dispatch, "TEST", "tests") action_id2 = processor.add_email_action(email_config_without_dispatch, "TEST", "tests") # Verify both were added successfully assert action_id1 is not None assert action_id2 is not None # Retrieve and verify dispatch_response settings actions = list(processor.list_email_action("TEST")) with_dispatch = next((a for a in actions if a['action_name'] == 'email_with_dispatch'), None) without_dispatch = next((a for a in actions if a['action_name'] == 'email_without_dispatch'), None) assert with_dispatch['dispatch_response'] is True assert without_dispatch['dispatch_response'] is FalseThis test would verify that:
- Actions can be created with both
TrueandFalsevalues fordispatch_response- The parameter is correctly stored and retrieved
🧰 Tools
🪛 Ruff (0.8.2)
15979-15979: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16032-16032: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16095-16096: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16095-16095: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16144-16144: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16217-16218: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16217-16217: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
16202-16203: Remove unnecessary empty linesThese empty lines don't serve any purpose and can be removed for better code readability.
email_config['to_email'] = temp - - def test_edit_email_action_does_not_exist(self):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/data_processor/data_processor_test.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15979-15979: 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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (7)
tests/unit_test/data_processor/data_processor_test.py (7)
15979-15979: LGTM - Don't worry about the unused variable warningThe static analysis tool flagged
mock_smtpas unused, but this is expected when using a context manager for mocking. The code correctly patches the SMTP class to prevent actual SMTP connections during the test.🧰 Tools
🪛 Ruff (0.8.2)
15979-15979: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16003-16030: Good test refactoring with clear documentationThe test refactoring includes clear comments explaining why the SMTP validation test is skipped and keeps the original test code for reference. This is a good practice for maintainability.
However, the email configuration is missing the new
dispatch_responseparameter that was added in this PR.
16045-16050: Good validation test for port rangeAdding validation for the SMTP port range is important for catching invalid configurations early. This test ensures that negative port numbers are rejected.
Consider also adding a test for values above 65535, which would also be invalid:
email_config['smtp_port'] = -1 with pytest.raises(ValidationError, match="Port must be between 0 and 65535"): processor.add_email_action(email_config, "TEST", "tests") + + email_config['smtp_port'] = 70000 + with pytest.raises(ValidationError, match="Port must be between 0 and 65535"): + processor.add_email_action(email_config, "TEST", "tests") email_config['smtp_port'] = temp
16099-16125: Well-structured test for duplicate action handlingThe test properly mocks both SMTP validation and the action name validation, focusing specifically on testing the duplicate action name handling. This is a good improvement over the previous version that had indentation issues.
However, the email configuration is missing the new
dispatch_responseparameter that was added in this PR.
16206-16209: Updated error message for better clarityThe updated error message format in the test provides more clarity about what went wrong. Good change!
Also applies to: 16218-16219
16223-16223: Updated count reflects additional email actionThe assertion correctly updated to expect 3 email actions instead of 2, which aligns with the changes mentioned in the PR summary.
15965-15981:Details
✅ Verification successful
New test missing the
dispatch_responseparameterThe test
test_add_email_action_with_custom_text_newdoesn't include the newdispatch_responseconfiguration parameter that was added according to the PR summary. This should be included to properly test the new functionality.email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, + "dispatch_response": True, "custom_text": {"value": "Hello from kairon!"} }Let's check if the
dispatch_responseparameter is referenced in the models or other tests:
🏁 Script executed:
#!/bin/bash # Look for dispatch_response parameter in models and other related files rg --type py "dispatch_response"Length of output: 13194
🏁 Script executed:
#!/bin/bash # Search for the implementation of add_email_action in the data processor rg -n "def add_email_action" -A 10 kairon/shared/data/processor.pyLength of output: 524
🏁 Script executed:
#!/bin/bash # Display implementation of add_email_action to check handling of dispatch_response sed -n '7084,7150p' kairon/shared/data/processor.pyLength of output: 2759
Confirm Default
dispatch_responseHandling inadd_email_actionThe
add_email_actionmethod explicitly does:action["dispatch_response"] = action.get("dispatch_response", True)– meaning, if the caller does not supply
dispatch_response, it will default toTrue. In your existing test, omittingdispatch_responsestill exercises the default-Truepath.Since the test only asserts that an ID is returned (and does not inspect the stored object’s
dispatch_response), adding"dispatch_response": Truetoemail_configwill not change its behavior or add coverage. If you want to verify the new parameter you should:
- Add an assertion after calling
add_email_actionthat retrieves the savedEmailActionConfigby name and checks its.dispatch_responsevalue.- (Optionally) Add a separate test setting
"dispatch_response": Falseinemail_configand assert that the saved config reflectsFalse.Without those assertions, simply including
"dispatch_response": Trueinemail_configis redundant.建议:
- Leave this test as‑is (it covers the default case).
- Add a new assertion or new test to explicitly verify both
TrueandFalsevalues ofdispatch_response.🧰 Tools
🪛 Ruff (0.8.2)
15979-15979: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
15984-15999: Good test addition for email action with custom text.This test case covers the scenario of adding an email action with custom text, which is a valuable addition to the test suite.
Consider adding an assertion to verify that the mock was called or use it in some way to address the static analysis warning:
with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: + mock_smtp.return_value.__enter__.return_value = mock_smtp assert processor.add_email_action(email_config, "TEST", "tests") is not None + assert mock_smtp.called🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(4 hunks)tests/unit_test/data_processor/data_processor_test.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration_test/services_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit_test/data_processor/data_processor_test.py (4)
tests/unit_test/utility_test.py (1)
bot(3273-3274)kairon/shared/data/processor.py (5)
add_schedule_action(8660-8695)list_schedule_action(8739-8755)add_email_action(7084-7107)edit_email_action(7109-7156)list_email_action(7158-7174)tests/unit_test/api/api_processor_test.py (1)
mock_smtp(1189-1190)tests/integration_test/services_test.py (1)
mock_smtp(115-116)
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (6)
tests/unit_test/data_processor/data_processor_test.py (6)
138-155: Re-enabling the schedule action test is a good addition.The test verifies the functionality of adding a schedule action with the
dispatch_bot_responseparameter set toTrue, which aligns with the email action's newdispatch_responseparameter. This ensures consistency in testing across different action types.
16022-16050: Improved test structure with better documentation.The changes to this test include clear documentation about why the SMTP validation is skipped, while preserving the original test case for reference. This is a good practice for maintaining test documentation.
16064-16068: Good addition of port validation test.This test case verifies that the
smtp_portvalue is properly validated to be within the range of 0 to 65535, which is an important validation for email configuration.
16118-16144: Well-structured test for duplicate action handling.This test effectively verifies that attempting to add an email action with a duplicate name raises the appropriate exception. The use of mocking to isolate the test case is well implemented.
16237-16237: Updated error message for better clarity.The error message has been updated to match the actual exception message thrown by the implementation, which improves test accuracy.
🧰 Tools
🪛 Ruff (0.8.2)
16236-16237: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16242-16242: Updated assertion to match current implementation.The assertion has been updated to check for 3 email actions instead of the previous value, which aligns with the current implementation state after adding the new test cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
16040-16050: Clean up commented test codeGood practice to skip the SMTP validation test that can't be reliably run. However, keeping commented code is generally not recommended as it clutters the codebase. Consider removing the commented section or adding a more concise comment explaining why it's skipped.
# Skip SMTP validation test due to system constraints pytest.skip("SMTP validation test skipped: Cannot trigger ValidationError for invalid SMTP URL without modifying system.") -# Original failing test case (kept for reference) -# with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: -# mock_smtp.side_effect = Exception("SMTP connection failed") -# with pytest.raises(ValidationError, match="Invalid SMTP url"): -# processor.add_email_action(email_config, "TEST", "tests") # Rest of the test cases remain unchanged
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(4 hunks)tests/unit_test/data_processor/data_processor_test.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration_test/services_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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 (6)
tests/unit_test/data_processor/data_processor_test.py (6)
138-149: Re-enabled test with proper dispatch_bot_response parameterGood to see this test re-enabled to verify schedule action functionality. It correctly includes the
dispatch_bot_responseparameter set toTrue, which aligns with the email dispatch flag feature being implemented.
4517-4517: Updated assertion count is consistent with added functionalityThe test now correctly expects 2 actions instead of 1, which is consistent with the changes made to add the dispatch flag functionality.
16064-16068: Good validation test for SMTP port rangeThe addition of a test case to validate the SMTP port range is a good practice to ensure proper validation of input parameters.
16118-16143: Well-structured test with proper mockingThis is a good implementation of the duplicate action test with proper mocking of both SMTP validation and the action name check. The test verifies that the appropriate exception is raised when attempting to add a duplicate action.
16237-16237: Updated exception message matches action nameThe exception message now correctly references "email_config5" to match the action name being tested.
🧰 Tools
🪛 Ruff (0.8.2)
16236-16237: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16242-16242: Updated assertion count reflects added email actionsThe test now correctly expects 3 email actions instead of 2, which is consistent with the changes made to add the dispatch flag functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
15987-16002: Missingdispatch_responseparameter in email_config.The test should include the new
dispatch_responseparameter which is the main focus of this PR. Additionally, the mock_smtp variable is assigned but never used.Apply this diff to fix both issues:
def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } - with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + # Add email action and verify dispatch_response was saved correctly + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify the dispatch_response parameter was correctly saved + from kairon.shared.data.models import EmailActionConfig + saved_action = EmailActionConfig.objects(name="email_config_with_custom_text_new", bot="TEST").get() + assert saved_action.dispatch_response is False🧰 Tools
🪛 Ruff (0.8.2)
16001-16001: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (2)
tests/unit_test/data_processor/data_processor_test.py (2)
16025-16028: Imports should be at the top of the file.Moving imports (
ValidationError,pytest, andpatch) inside the class body is not a recommended Python practice. These imports should be at the top of the file.- from mongoengine.errors import ValidationError - import pytest - from unittest.mock import patch + # These imports should be moved to the top of the file
16044-16052: Consider removing commented-out code.The commented-out test case adds clutter and could be removed since it's no longer needed. If you want to keep it for reference, consider adding a more detailed comment explaining why it's retained.
# Skip SMTP validation test due to system constraints pytest.skip("SMTP validation test skipped: Cannot trigger ValidationError for invalid SMTP URL without modifying system.") - # Original failing test case (kept for reference) - # with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - # mock_smtp.side_effect = Exception("SMTP connection failed") - # with pytest.raises(ValidationError, match="Invalid SMTP url"): - # processor.add_email_action(email_config, "TEST", "tests")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(4 hunks)tests/unit_test/data_processor/data_processor_test.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration_test/services_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
16001-16001: 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 (4)
- GitHub Check: CodeQL
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
tests/unit_test/data_processor/data_processor_test.py (6)
138-155: LGTM! Test re-enabled properly.This test for adding a schedule action follows a good structure with clear setup, action, and verification steps.
4517-4521: Assertion logic improved!Good refactoring to extract action names and make specific assertions about the expected actions.
16067-16071: LGTM! Good validation test for port range.This test correctly validates that the smtp_port must be between 0 and 65535.
16121-16147: LGTM! Well-structured test for duplicate action handling.This test properly mocks the dependencies and verifies the exception raised when adding a duplicate email action.
16240-16240: LGTM! Updated exception message.Good update to match the new format of the exception message.
🧰 Tools
🪛 Ruff (0.8.2)
16239-16240: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16245-16245: Updated assertion for email action count.Updated to expect 3 email actions, correctly reflecting the changes made in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
15984-15999: 💡 Verification agent🧩 Analysis chain
Add the new dispatch_response parameter to email action test.
The test for adding an email action with custom text doesn't include the new
dispatch_responseparameter that's the focus of this PR. Additionally, the mock_smtp variable is assigned but never used.email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } -with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None +# No need to mock SMTP as add_email_action only persists the data +action_id = processor.add_email_action(email_config, "TEST", "tests") +assert action_id is not None + +# Verify the dispatch_response parameter was correctly saved +from kairon.shared.data.data_models import EmailActionConfig +saved_action = EmailActionConfig.objects(id=action_id).get() +assert saved_action.dispatch_response is FalseLet's check if our test coverage approach is feasible:
🏁 Script executed:
#!/bin/bash # Check the structure of EmailActionConfig to confirm dispatch_response field rg -n "class EmailActionConfig" -A20 kairon/shared/data/data_models.py # Check where dispatch_response is set in processor.py rg -n "dispatch_response" kairon/shared/data/processor.pyLength of output: 964
I want to confirm where and how
EmailActionConfigis defined to ensure thedispatch_responsefield exists on the model.
🏁 Script executed:
#!/bin/bash # Search for the EmailActionConfig class definition across the repo rg -n "class EmailActionConfig" -A5 . # If that yields nothing, locate any reference to EmailActionConfig rg -n "EmailActionConfig" --glob "*.py"Length of output: 7155
To confirm that the
dispatch_responsefield exists onEmailActionConfig, let’s inspect its full definition.
🏁 Script executed:
#!/bin/bash # Display the EmailActionConfig class fields to verify dispatch_response is declared sed -n '430,480p' kairon/shared/actions/data_objects.pyLength of output: 2632
Ensure
dispatch_responseis included and remove unused SMTP mockThe
EmailActionConfigmodel definesdispatch_response = BooleanField(default=True), andadd_email_actionusesaction_config.get("dispatch_response", True). We should update the test to:
- Add
"dispatch_response"inemail_config.- Drop the
SMTPmock sinceadd_email_actiononly persists data.- Import
EmailActionConfigfromkairon.shared.actions.data_objects.Suggested test update:
def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() - email_config = {"action_name": "...", + email_config = { + "action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, ... - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False + } - with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify dispatch_response was saved correctly + from kairon.shared.actions.data_objects import EmailActionConfig + saved = EmailActionConfig.objects(id=action_id).get() + assert saved.dispatch_response is False
- Location to update:
tests/unit_test/data_processor/data_processor_test.py, thetest_add_email_action_with_custom_text_newmethod.🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
16022-16025: Improve test structure and organization.The imports for
ValidationError,pytest, andpatchshould be moved to the top of the file for better organization.The skipped test case is properly documented with a clear explanation about why it's skipped, which is good practice.
-from mongoengine.errors import ValidationError -import pytest -from unittest.mock import patch +# Move these imports to the top of the fileAlso applies to: 16041-16049
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/data_processor/data_processor_test.py(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit_test/data_processor/data_processor_test.py (4)
tests/unit_test/utility_test.py (1)
bot(3273-3274)kairon/shared/data/processor.py (5)
add_schedule_action(8660-8695)list_schedule_action(8739-8755)add_email_action(7084-7107)edit_email_action(7109-7156)list_email_action(7158-7174)tests/unit_test/api/api_processor_test.py (1)
mock_smtp(1189-1190)tests/integration_test/services_test.py (1)
mock_smtp(115-116)
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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 (6)
tests/unit_test/data_processor/data_processor_test.py (6)
138-155: Well-structured test case for schedule actions.The re-enabled
test_add_schedule_action_alooks good. It properly tests the addition of a schedule action and verifies the action is correctly stored in the database.
4517-4518: Assertions correctly updated to match new functionality.The updated assertion correctly checks for the expected number of actions (2) and verifies that the first action is named 'test_schedule_action'.
16064-16068: Good test case for port validation.The added test for validating that port values must be between 0 and 65535 is a solid addition that ensures the validation logic works correctly.
16118-16144: Improved test implementation for duplicate action detection.The rewritten test for duplicate email actions is much better than the previous implementation. It:
- Properly mocks SMTP validation
- Mocks the action name validation to simulate a duplicate
- Checks for the expected exception
This approach is more focused and reliable than the previous implementation.
16225-16225: Updated test for non-existent action.The test correctly checks that attempting to edit a non-existent email action raises the expected exception with the appropriate message.
Also applies to: 16237-16237
16242-16242: Updated assertion for email action count.The assertion update to check for 3 email actions (instead of the previous count) correctly reflects the changes to the test setup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration_test/services_test.py (1)
12181-12284: The restored test logic is sound, but consider using the equality operator directly.This test properly validates that story names with special characters are rejected with the appropriate error code. However, the assertion syntax could be improved.
- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"Also, consider removing the debug print statement on line 12280 before merging to production.
🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration_test/services_test.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
⏰ 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 (4)
tests/integration_test/services_test.py (4)
10132-10136: Formatting change looks good.This change simply adjusts the whitespace between test functions, which improves readability.
23049-23053: Formatting change looks good.This change simply adjusts the whitespace between email action test functions, which improves readability.
30398-30410: Formatting change looks good.This change adds appropriate whitespace around the API call in the test, which improves readability.
12180-12285: Verify test coverage for the new email dispatch_response parameter.While restoring this test for invalid story names is good, I don't see tests explicitly verifying the new
dispatch_responseparameter mentioned in the PR summary.Consider adding test cases that verify:
- Email actions with
dispatch_response=Truecorrectly dispatch responses- Email actions with
dispatch_response=Falsedon't dispatch responses- Proper validation of the
dispatch_responseparameter during action creation/updating🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/integration_test/services_test.py (1)
23298-23301: Test case removal requires justification.This test for listing email actions is being removed without explanation. If it's being replaced by tests elsewhere (such as in unit tests), please document this.
#!/bin/bash # Verify if equivalent test coverage exists in unit tests echo "Checking for email action list tests in unit tests directory..." rg "test_list_email_actions|test.*list.*email" tests/unit_test/ -ltests/unit_test/data_processor/data_processor_test.py (1)
15984-15999: 🛠️ Refactor suggestionAdd dispatch_response parameter to the email configuration test
This test doesn't include the new
dispatch_responseparameter that's the focus of this PR. Also, themock_smtpvariable is assigned but never used.def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } - with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify the dispatch_response parameter was correctly saved + action = EmailActionConfig.objects(name="email_config_with_custom_text_new", bot="TEST").get() + assert action.dispatch_response is False🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (3)
tests/integration_test/services_test.py (2)
12181-12284: Restored test for invalid story name validation with improper assertion syntax.The test correctly verifies that multiflow stories with invalid characters are rejected with a 422 error code. However, there's an issue with the assertion syntax.
- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"Using
!=is more idiomatic Python thannot ... ==and improves code readability.🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
12280-12280: Remove debug print statement.Remove the debug print statement to keep the test output clean.
- print(actual)tests/unit_test/data_processor/data_processor_test.py (1)
16022-16025: Move import statements to the top of the fileImport statements should be placed at the top of the file, not between test methods, to improve code organization and readability.
These import statements should be moved to the top of the file with other imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(5 hunks)tests/unit_test/data_processor/data_processor_test.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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: Analyze (python)
- GitHub Check: Python CI
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
tests/integration_test/services_test.py (3)
10132-10138: Improved code formatting with standard spacing.The formatting change maintains Python's convention of having a single blank line between test functions, improving readability.
23049-23055: Improved code spacing between test methods.The additional blank line improves readability by clearly separating test functions.
30469-30483: Improved readability with consistent spacing.The additional blank lines create better visual separation between different steps in the test case, improving readability.
tests/unit_test/data_processor/data_processor_test.py (6)
16056-16061: LGTM: New validation for SMTP port rangeThe addition of a validation test case for SMTP port range is a good enhancement to ensure the port is within valid bounds (0-65535).
16110-16136: LGTM: Improved email action duplicate testThis is a well-structured test that properly mocks the SMTP validation and the action name validation to test the duplicate action name scenario. The test now correctly validates that an
AppExceptionwith the message "Action name already exists!" is raised when attempting to add a duplicate email action.
16217-16217: LGTM: Updated action name in testThe action name and exception message have been properly updated to test for a non-existent email action.
Also applies to: 16229-16229
16234-16234: LGTM: Updated email actions count assertionThe assertion has been updated to expect 3 email actions, which is consistent with the changes made to add more email action tests.
138-155: LGTM: Re-enabled schedule action testSuccessfully re-enabled the test for adding a schedule action with proper assertions to verify the action was added correctly.
4517-4518: LGTM: Updated action count and name assertionsThe test has been updated to assert the presence of 2 actions and specifically checks that the first action is named 'test_schedule_action', which aligns with the re-enabled test.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/integration_test/services_test.py (1)
23298-23307: Re-added email actions list test.Good job restoring the previously removed test case for email actions listing. This test is important for verifying the email actions functionality, particularly with the new dispatch_response parameter.
Note that the expected count of email actions has been updated from 3 to 4, which aligns with the changes in the email action functionality.
tests/unit_test/data_processor/data_processor_test.py (1)
15984-15999: 💡 Verification agent🧩 Analysis chain
Add the new
dispatch_responseparameter to the test.This test doesn't include the new
dispatch_responseparameter that's the focus of this PR. Also, themock_smtpvariable is defined but never used in the test.def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } - with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify the dispatch_response parameter was correctly saved + action = processor.get_action("email_config_with_custom_text_new", "TEST") + assert action["dispatch_response"] is FalseLet's verify if the
get_actionmethod is available:
🏁 Script executed:
#!/bin/bash # Check if the get_action method exists in the processor rg "def get_action" --type pyLength of output: 700
🏁 Script executed:
#!/bin/bash # Locate the definition of MongoProcessor and inspect its methods file="kairon/shared/data/processor.py" start_line=$(rg -n "class MongoProcessor" --type py "$file" | cut -d: -f1) # Print the class definition and subsequent methods for inspection sed -n "${start_line},$((start_line+300))p" "$file"Length of output: 11142
🏁 Script executed:
#!/bin/bash # Find the definition of add_email_action in the processor file rg -n "def add_email_action" --type py kairon/shared/data/processor.pyLength of output: 144
🏁 Script executed:
#!/bin/bash # Find get_actions method in MongoProcessor rg -n "def get_actions" --type py kairon/shared/data/processor.py line=$(rg -n "def get_actions" --type py kairon/shared/data/processor.py | cut -d: -f1) sed -n "${line},$((line+50))p" kairon/shared/data/processor.pyLength of output: 2065
🏁 Script executed:
#!/bin/bash file="kairon/shared/data/processor.py" # Print add_email_action implementation rg -n "def add_email_action" --type py "$file" | cut -d: -f1 | while read l; do echo "=== add_email_action (around line $l) ===" sed -n "$((l-2)),$((l+50))p" "$file" done # Print __prepare_document_list implementation rg -n "def __prepare_document_list" --type py "$file" | cut -d: -f1 | while read l; do echo "=== __prepare_document_list (around line $l) ===" sed -n "$((l-2)),$((l+50))p" "$file" doneLength of output: 4528
Update test to include
dispatch_responseand remove unused SMTP mockThe test should explicitly set and verify the new
dispatch_responsefield, and sinceMongoProcessordoesn’t expose aget_actionmethod, fetch the saved config via theEmailActionConfigmodel. Also remove the unused SMTP patch.Key changes:
- Add
"dispatch_response": Falseto theemail_configdict.- Remove the
with patch("kairon.shared.utils.SMTP", …)block.- After calling
add_email_action, assert the returned ID is notNone, then importEmailActionConfigto retrieve the saved document and assert itsdispatch_responsevalue.Updated diff:
def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() email_config = { "action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {"value": "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } - with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify dispatch_response was persisted correctly + from kairon.shared.data.processor import EmailActionConfig + saved_action = EmailActionConfig.objects( + action_name="email_config_with_custom_text_new", + bot="TEST", status=True + ).get() + assert saved_action.dispatch_response is False🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (1)
tests/integration_test/services_test.py (1)
12181-12285: Restored test case for validation of multiflow story names.Great job re-enabling this test case to verify that invalid story names are properly rejected. This helps ensure data validation remains robust.
Line 12284 can be simplified for better readability:
- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(5 hunks)tests/unit_test/data_processor/data_processor_test.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: Local variable mock_smtp is assigned to but never used
Remove assignment to unused variable mock_smtp
(F841)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
⏰ 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 (10)
tests/integration_test/services_test.py (3)
10135-10135: Code formatting improvement.Added blank line improves readability by properly separating test assertions from the next test function declaration.
23052-23052: Code formatting improvement.Added blank line improves readability by properly separating test functions.
30472-30472: Improved code spacing.Added blank lines enhance readability by properly separating different sections of the test code.
Also applies to: 30480-30480
tests/unit_test/data_processor/data_processor_test.py (7)
138-155: Re-enabled test adds necessary test coverage.The re-enabled
test_add_schedule_action_atest creates data required for subsequent tests and helps validate the schedule action functionality. This is important for comprehensive test coverage.
4517-4518: Updated assertions match the re-enabled test.Good update to the
test_get_actionsassertions. The test now correctly expects 2 actions (including the one created bytest_add_schedule_action_a) and verifies the name of the first action.
16040-16040: Improved exception message specificity.Good change to make the mocked exception message more specific ("Invalid SMTP url"). This makes the test more precise about the kind of error it's expecting.
16056-16060: Added validation for port range.Good addition of a test case to verify that port values must be within the valid range of 0-65535.
16217-16217: Action name updated consistently.Good update to the action name in the test. This ensures the test correctly verifies that an action with this specific name doesn't exist.
16229-16229: Error message updated to match the new action name.Good update to the expected error message to reflect the changed action name from "email_config1" to "email_config5".
🧰 Tools
🪛 Ruff (0.8.2)
16228-16229: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16234-16234: Updated assertion for email action count.Good update to expect 3 email actions instead of the previous count. This reflects the additional email action created in the new test.
| def test_add_email_action_duplicate(self): | ||
| processor = MongoProcessor() | ||
| email_config = { | ||
| "action_name": "email_config1", | ||
| "smtp_url": "smtp.gmail.com", | ||
| "smtp_port": 587, | ||
| "smtp_userid": None, | ||
| "smtp_password": {'value': "test"}, | ||
| "from_email": {"value": "[email protected]", "parameter_type": "value"}, | ||
| "to_email": {"value": "to_email", "parameter_type": "slot"}, | ||
| "subject": "Test Subject", | ||
| "response": "Test Response", | ||
| "tls": False | ||
| } | ||
|
|
||
| # Mock successful SMTP validation | ||
| with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: | ||
| mock_smtp.return_value = None # Simulate valid SMTP configuration | ||
|
|
||
| # Mock Utility.is_valid_action_name to raise AppException for duplicate action | ||
| with patch("kairon.shared.utils.Utility.is_valid_action_name") as mock_valid_action: | ||
| mock_valid_action.side_effect = AppException("Action name already exists!") | ||
|
|
||
| # Test duplicate action name raises AppException | ||
| with pytest.raises(AppException, match="Action name already exists!"): | ||
| processor.add_email_action(email_config, "TEST", "tests") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include the new dispatch_response parameter in email configuration.
The test is well structured with proper mocking, but doesn't include the new dispatch_response parameter that's the focus of this PR.
email_config = {
"action_name": "email_config1",
"smtp_url": "smtp.gmail.com",
"smtp_port": 587,
"smtp_userid": None,
"smtp_password": {'value': "test"},
"from_email": {"value": "[email protected]", "parameter_type": "value"},
"to_email": {"value": "to_email", "parameter_type": "slot"},
"subject": "Test Subject",
"response": "Test Response",
- "tls": False
+ "tls": False,
+ "dispatch_response": True
}📝 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.
| def test_add_email_action_duplicate(self): | |
| processor = MongoProcessor() | |
| email_config = { | |
| "action_name": "email_config1", | |
| "smtp_url": "smtp.gmail.com", | |
| "smtp_port": 587, | |
| "smtp_userid": None, | |
| "smtp_password": {'value': "test"}, | |
| "from_email": {"value": "[email protected]", "parameter_type": "value"}, | |
| "to_email": {"value": "to_email", "parameter_type": "slot"}, | |
| "subject": "Test Subject", | |
| "response": "Test Response", | |
| "tls": False | |
| } | |
| # Mock successful SMTP validation | |
| with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: | |
| mock_smtp.return_value = None # Simulate valid SMTP configuration | |
| # Mock Utility.is_valid_action_name to raise AppException for duplicate action | |
| with patch("kairon.shared.utils.Utility.is_valid_action_name") as mock_valid_action: | |
| mock_valid_action.side_effect = AppException("Action name already exists!") | |
| # Test duplicate action name raises AppException | |
| with pytest.raises(AppException, match="Action name already exists!"): | |
| processor.add_email_action(email_config, "TEST", "tests") | |
| def test_add_email_action_duplicate(self): | |
| processor = MongoProcessor() | |
| email_config = { | |
| "action_name": "email_config1", | |
| "smtp_url": "smtp.gmail.com", | |
| "smtp_port": 587, | |
| "smtp_userid": None, | |
| "smtp_password": {'value': "test"}, | |
| "from_email": {"value": "[email protected]", "parameter_type": "value"}, | |
| "to_email": {"value": "to_email", "parameter_type": "slot"}, | |
| "subject": "Test Subject", | |
| "response": "Test Response", | |
| "tls": False, | |
| "dispatch_response": True | |
| } | |
| # Mock successful SMTP validation | |
| with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: | |
| mock_smtp.return_value = None # Simulate valid SMTP configuration | |
| # Mock Utility.is_valid_action_name to raise AppException for duplicate action | |
| with patch("kairon.shared.utils.Utility.is_valid_action_name") as mock_valid_action: | |
| mock_valid_action.side_effect = AppException("Action name already exists!") | |
| # Test duplicate action name raises AppException | |
| with pytest.raises(AppException, match="Action name already exists!"): | |
| processor.add_email_action(email_config, "TEST", "tests") |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/unit_test/data_processor/data_processor_test.py (2)
15984-15999: 🛠️ Refactor suggestionInclude the new
dispatch_responseparameter in the testThe test doesn't include the new
dispatch_responseparameter which is the focus of this PR. Also, the mock_smtp variable is assigned but never used.email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify the dispatch_response parameter was correctly saved + action = EmailActionConfig.objects(id=action_id).get() + assert action.dispatch_response is False🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
16110-16136: 🛠️ Refactor suggestionInclude the new
dispatch_responseparameter in email configurationThe test is well structured with proper mocking, but doesn't include the new
dispatch_responseparameter that's the focus of this PR.email_config = { "action_name": "email_config1", "smtp_url": "smtp.gmail.com", "smtp_port": 587, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "[email protected]", "parameter_type": "value"}, "to_email": {"value": "to_email", "parameter_type": "slot"}, "subject": "Test Subject", "response": "Test Response", - "tls": False + "tls": False, + "dispatch_response": True }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/data_processor/data_processor_test.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (7)
tests/unit_test/data_processor/data_processor_test.py (7)
138-155: Test method re-enabled with proper implementationGood to see this test method being re-enabled. It correctly validates the creation of a schedule action and the presence of the
dispatch_bot_responseparameter. Consider enhancing the test to validate more properties beyond just the name.
4517-4518: LGTM - assertion count updated correctlyThe assertion count has been updated to reflect the additional test_schedule_action added when re-enabling the test_add_schedule_action_a method.
16040-16040: Improved exception message specificityGood improvement to specify the exact error message rather than using a generic exception. This makes the test more precise and easier to understand.
16056-16061: Good validation test for SMTP portThis is a good addition to test port validation. It correctly verifies that negative port values are rejected.
Consider also testing the upper boundary (port > 65535) to ensure complete coverage of the validation logic.
16217-16217: Action name updated for test integrityGood update to use a different action name to ensure test isolation and prevent conflicts with other tests.
16229-16229: Exception message updated to match action nameGood update to ensure the exception message matches the new action name.
🧰 Tools
🪛 Ruff (0.8.2)
16228-16229: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16234-16234: Expected email action count updatedThe updated count reflects the additional email actions being created in the tests.
| from mongoengine.errors import ValidationError | ||
| import pytest | ||
| from unittest.mock import patch | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move imports to the top of the file
Imports should be placed at the top of the file rather than in the middle of the code. This improves readability and follows standard Python coding conventions.
These import statements should be moved to the top of the file with other imports:
from mongoengine.errors import ValidationError
import pytest
from unittest.mock import patch
|
|
||
|
|
||
| def test_list_email_actions(): | ||
| @patch("kairon.shared.utils.SMTP", autospec=True) |
There was a problem hiding this comment.
remove unwanted patch
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/integration_test/services_test.py (1)
23298-23301: Restored email actions test case with proper mockingGood job restoring the
test_list_email_actionstest function with appropriate SMTP mocking. This addresses the previous review comment "why removed this testcase?".tests/unit_test/data_processor/data_processor_test.py (3)
16022-16025: Move imports to the top of the file.These import statements should be located at the top of the file with other imports, not in the middle of the code. This improves readability and follows Python conventions.
-from mongoengine.errors import ValidationError -import pytest -from unittest.mock import patchMove these to the imports section at the top of the file.
16104-16130: Add the newdispatch_responseparameter to the test configuration.This new test for checking duplicate email actions is missing the
dispatch_responseparameter that is the focus of this PR.email_config = { "action_name": "email_config1", "smtp_url": "smtp.gmail.com", "smtp_port": 587, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "[email protected]", "parameter_type": "value"}, "to_email": {"value": "to_email", "parameter_type": "slot"}, "subject": "Test Subject", "response": "Test Response", - "tls": False + "tls": False, + "dispatch_response": True }Otherwise, the test is well-structured with proper mocking of both SMTP validation and action name validation.
15984-15999: 🛠️ Refactor suggestionAdd the new
dispatch_responseparameter to test the feature properly.This new test for email actions with custom text is missing the
dispatch_responseparameter, which is the primary feature added in this PR. Additionally, themock_smtpvariable is assigned but never used.def test_add_email_action_with_custom_text_new(self): processor = MongoProcessor() email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": False } with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + # Verify the dispatch_response parameter was correctly saved + action = EmailActionConfig.objects(id=action_id).get() + assert action.dispatch_response is False🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (1)
tests/integration_test/services_test.py (1)
12284-12284: Use!=operator instead ofnot ... ==The current assertion uses a less readable form with
not actual["message"] == "valid story name".- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(5 hunks)tests/unit_test/data_processor/data_processor_test.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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 (11)
tests/integration_test/services_test.py (4)
10135-10135: LGTM: Added proper spacing before test functionThe additional blank line improves readability by properly separating test functions.
12181-12284: Restored and updated test case for invalid multiflow story nameGood job restoring this test case that validates proper handling of invalid story names with special characters. This ensures proper input validation and error handling.
🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
23052-23052: LGTM: Added proper spacing between test functionsThe additional blank line improves readability by properly separating test functions.
30471-30471: LGTM: Added proper spacing for improved readabilityThe additional blank lines improve code readability by properly separating logical blocks within the test.
Also applies to: 30479-30479
tests/unit_test/data_processor/data_processor_test.py (7)
4517-4518: LGTM on the updated assertions.The changes correctly verify that the actions list has exactly 2 entries and that the first action has the name 'test_schedule_action', which is consistent with the updated functionality.
16040-16040: LGTM on the updated exception message.The test now correctly mocks the SMTP side effect with the specific error message "Invalid SMTP url" that matches the implementation.
16085-16085: LGTM on the formatting improvement.Adding an empty line between test methods improves code readability.
16207-16208: LGTM on the improved formatting.Adding empty lines between test methods improves code readability.
16211-16211: LGTM on the updated action name.Changing the action name to "email_config5" helps ensure this test is isolated from other test cases.
16223-16223: LGTM on the updated error message.The test's expected exception message now correctly matches the specific action name being tested.
🧰 Tools
🪛 Ruff (0.8.2)
16222-16223: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16228-16228: LGTM on the updated assertion.The test now correctly verifies that there are 4 email actions, which is consistent with the additional test cases that have been added.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/unit_test/data_processor/data_processor_test.py (2)
16104-16130: 🛠️ Refactor suggestionImproved test for duplicate email action handling.
This test properly handles the duplicate email action case by mocking SMTP validation and the action name validation, which is more robust than the previous implementation.
However, the email configuration is missing the new
dispatch_responseparameter:email_config = { "action_name": "email_config1", "smtp_url": "smtp.gmail.com", "smtp_port": 587, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "[email protected]", "parameter_type": "value"}, "to_email": {"value": "to_email", "parameter_type": "slot"}, "subject": "Test Subject", "response": "Test Response", - "tls": False + "tls": False, + "dispatch_response": True }
15984-15999: 🛠️ Refactor suggestionAdd the new
dispatch_responseparameter to the email config.This test for adding an email action with custom text is missing the new
dispatch_responseparameter that is the focus of this PR.email_config = {"action_name": "email_config_with_custom_text_new", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {'value': "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, - "custom_text": {"value": "Hello from kairon!"} + "custom_text": {"value": "Hello from kairon!"}, + "dispatch_response": True }Also, the mock_smtp variable is assigned but never used. Consider either using the mock or removing the unnecessary patch:
-with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: +with patch("kairon.shared.utils.SMTP", autospec=True): assert processor.add_email_action(email_config, "TEST", "tests") is not NoneOr better yet, verify the saved configuration:
-with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: - assert processor.add_email_action(email_config, "TEST", "tests") is not None +with patch("kairon.shared.utils.SMTP", autospec=True): + action_id = processor.add_email_action(email_config, "TEST", "tests") + assert action_id is not None + + # Verify the dispatch_response parameter was correctly saved + from kairon.shared.data.processor import EmailActionConfig + saved_action = EmailActionConfig.objects(id=action_id).get() + assert saved_action.dispatch_response is True🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable
mock_smtpis assigned to but never usedRemove assignment to unused variable
mock_smtp(F841)
🧹 Nitpick comments (1)
tests/unit_test/data_processor/data_processor_test.py (1)
16022-16025: Move imports to the top of the file.These import statements should be at the top of the file with other imports, following Python's PEP 8 style guide.
-from mongoengine.errors import ValidationError -import pytest -from unittest.mock import patchThese lines should be moved to the top of the file where other imports are located.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/data_processor/data_processor_test.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (7)
tests/unit_test/data_processor/data_processor_test.py (7)
4518-4518: Updated assertion reflects change in expected action name.The test now expects the first action to be named 'test_schedule_action' instead of a previous value, which aligns with changes made in the implementation.
16040-16040: Updated exception message for better error handling.The specific exception message "Invalid SMTP url" makes the test more precise and matches the expected validation error.
16085-16085: Extra blank line added.This is just a formatting change to improve readability and separate test methods.
16207-16208: Extra blank lines added.This is just a formatting change to improve readability and separate test methods.
16211-16211: Updated non-existent action name in test.The test now checks for "email_config5" instead of a previous action name, likely to avoid conflicts with other tests.
16223-16223: Updated error message to match the new action name.The error message now correctly references "email_config5", matching the updated action name in the test.
🧰 Tools
🪛 Ruff (0.8.2)
16222-16223: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16228-16228: Updated assertion for the number of email actions.The test now expects 3 email actions instead of a previous value, likely because additional email actions were added as part of the changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/integration_test/services_test.py (1)
23298-23300: Re-added email actions list test with proper SMTP mocking.Good to see this test restored as it's important for the email action functionality, especially with the new
dispatch_responseparameter. The SMTP patch is necessary for proper mocking of email functionality.This addresses previous review comments that questioned why this test was removed.
tests/unit_test/data_processor/data_processor_test.py (2)
16022-16025: Move imports to the top of the fileImports should be placed at the top of the file rather than in the middle of the code. This improves readability and follows standard Python coding conventions.
16104-16130: Include thedispatch_responseparameter in the email configuration.The test is well structured with proper mocking, but doesn't include the new
dispatch_responseparameter that's the focus of this PR.
🧹 Nitpick comments (4)
tests/integration_test/services_test.py (4)
10135-10135: Consistent whitespace formatting applied.The addition of this blank line improves code readability by maintaining consistent spacing between test functions.
12181-12284: Good to see the test for invalid story names restored.This test validates the proper handling of invalid characters in story names, ensuring the API correctly rejects them with a 422 error.
However, there's a minor syntax improvement suggestion:
- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"This makes the assertion more readable and directly conveys the intention.
🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
23052-23052: Consistent whitespace formatting applied.The addition of this blank line improves readability by maintaining consistent spacing between test functions.
30472-30472: Improved test readability with consistent spacing.The addition of blank lines around the client request and response assertion blocks improves code readability by clearly separating logical sections of the test.
Also applies to: 30480-30480
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_test/services_test.py(5 hunks)tests/unit_test/data_processor/data_processor_test.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
tests/unit_test/data_processor/data_processor_test.py
15998-15998: 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 (5)
tests/unit_test/data_processor/data_processor_test.py (5)
16040-16040: Good implementation of SMTP error mocking.The mock now correctly simulates an SMTP failure with a specific error message that matches the expected exception.
16104-16130: Well-implemented test for duplicate action name detection.The test now correctly uses multiple mocks to thoroughly test the duplicate action scenario. The improved indentation fixes the previous issue, making this a separate test method.
16211-16211: Updated action name in test_edit_email_action_does_not_exist.The action name has been updated from "email_config1" to "email_config5".
16223-16223: Correctly updated exception message in test_edit_email_action_does_not_exist.The exception match string has been properly updated to include the new action name "email_config5".
🧰 Tools
🪛 Ruff (0.8.2)
16222-16223: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
16228-16228: Updated assertion for the number of email actions.The assertion now correctly expects 3 email actions instead of 2, reflecting changes in the codebase.
| def test_add_email_action_with_custom_text_new(self): | ||
| processor = MongoProcessor() | ||
| email_config = {"action_name": "email_config_with_custom_text_new", | ||
| "smtp_url": "test.test.com", | ||
| "smtp_port": 25, | ||
| "smtp_userid": None, | ||
| "smtp_password": {'value': "test"}, | ||
| "from_email": {"value": "from_email", "parameter_type": "slot"}, | ||
| "to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"}, | ||
| "subject": "Test Subject", | ||
| "response": "Test Response", | ||
| "tls": False, | ||
| "custom_text": {"value": "Hello from kairon!"} | ||
| } | ||
| with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp: | ||
| assert processor.add_email_action(email_config, "TEST", "tests") is not None |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Include the dispatch_response parameter in the email configuration test.
The test is well-structured but doesn't include the new dispatch_response parameter that's the focus of this PR. Additionally, the mock_smtp variable is assigned but never used.
email_config = {"action_name": "email_config_with_custom_text_new",
"smtp_url": "test.test.com",
"smtp_port": 25,
"smtp_userid": None,
"smtp_password": {'value': "test"},
"from_email": {"value": "from_email", "parameter_type": "slot"},
"to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"},
"subject": "Test Subject",
"response": "Test Response",
"tls": False,
- "custom_text": {"value": "Hello from kairon!"}
+ "custom_text": {"value": "Hello from kairon!"},
+ "dispatch_response": True
}
with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp:
- assert processor.add_email_action(email_config, "TEST", "tests") is not None
+ action_id = processor.add_email_action(email_config, "TEST", "tests")
+ assert action_id is not None
+
+ # Verify the dispatch_response parameter was correctly saved
+ saved_action = processor.get_email_action("email_config_with_custom_text_new", "TEST")
+ assert saved_action["dispatch_response"] is True🏁 Script executed:
#!/bin/bash
# Check if dispatch_response is part of EmailActionConfig model
rg -n -A 5 "class EmailActionConfig" kairon/shared/actions/data_objects.pyLength of output: 421
🏁 Script executed:
#!/bin/bash
# Search for usage of dispatch_response in the model and entire codebase
rg -n "dispatch_response" kairon/shared/actions/data_objects.py
rg -n "dispatch_response" .Length of output: 15501
Include dispatch_response in the email‐action test and assert SMTP mock usage
The test_add_email_action_with_custom_text_new helper is missing the new dispatch_response flag, and the patched SMTP client (mock_smtp) is never exercised. Please update the test to:
- Add
"dispatch_response": Trueto theemail_config. - Capture and assert the returned action ID.
- Verify that your SMTP client was initialized with the right host/port via
mock_smtp. - (Optionally) Load back the saved action via
processor.get_email_action(...)and confirm itsdispatch_responsevalue.
Locations to update:
- tests/unit_test/data_processor/data_processor_test.py (around
test_add_email_action_with_custom_text_new)
Suggested diff:
def test_add_email_action_with_custom_text_new(self):
processor = MongoProcessor()
- email_config = {"action_name": "email_config_with_custom_text_new",
+ email_config = {"action_name": "email_config_with_custom_text_new",
"smtp_url": "test.test.com",
"smtp_port": 25,
"smtp_userid": None,
"smtp_password": {'value': "test"},
"from_email": {"value": "from_email", "parameter_type": "slot"},
"to_email": {"value": ["[email protected]", "[email protected]"], "parameter_type": "value"},
"subject": "Test Subject",
"response": "Test Response",
"tls": False,
- "custom_text": {"value": "Hello from kairon!"}
+ "custom_text": {"value": "Hello from kairon!"},
+ "dispatch_response": True
}
with patch("kairon.shared.utils.SMTP", autospec=True) as mock_smtp:
- assert processor.add_email_action(email_config, "TEST", "tests") is not None
+ action_id = processor.add_email_action(email_config, "TEST", "tests")
+ assert action_id is not None
+
+ # Verify SMTP initialization
+ mock_smtp.assert_called_once_with("test.test.com", 25)
+
+ # Optionally verify that dispatch_response was persisted
+ saved = processor.get_email_action("email_config_with_custom_text_new", "TEST")
+ assert saved["dispatch_response"] is TrueCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
15998-15998: Local variable mock_smtp is assigned to but never used
Remove assignment to unused variable mock_smtp
(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration_test/services_test.py (1)
23298-23300: Restored email actions test with SMTP mocking.Excellent decision to restore this test case. The SMTP mocking is appropriately implemented to test email action functionality.
🧹 Nitpick comments (1)
tests/integration_test/services_test.py (1)
12284-12284: Use!=operator instead ofnot ... ==for better readability.The static analysis tool correctly identified that the assertion can be simplified.
- assert not actual["message"] == "valid story name" + assert actual["message"] != "valid story name"🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration_test/services_test.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/services_test.py
12284-12284: Use actual["message"] != "valid story name" instead of not actual["message"] == "valid story name"
Replace with != operator
(SIM201)
⏰ 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 (7)
tests/integration_test/services_test.py (7)
10135-10135: LGTM: Appropriate spacing added between test functions.The addition of a blank line improves readability by properly separating test functions.
12181-12284: Restored test for invalid story name validation.Good to see this test function being restored. It properly tests that story names containing special characters are rejected with the appropriate error code.
🧰 Tools
🪛 Ruff (0.8.2)
12284-12284: Use
actual["message"] != "valid story name"instead ofnot actual["message"] == "valid story name"Replace with
!=operator(SIM201)
23052-23052: LGTM: Consistent spacing added between test functions.The addition of a blank line improves readability by properly separating test functions.
23321-23329: Added dispatch_response parameter to email configuration.The new
dispatch_responseparameter has been correctly added to the email action configuration, which aligns with the PR objective of introducing an email dispatch flag.
23341-23348: Added dispatch_response parameter to slot-based email configuration.The new
dispatch_responseparameter has been correctly added to the email action configuration that uses slots, ensuring consistency across different parameter types.
23368-23368: Added dispatch_response parameter to key vault email configuration.The new
dispatch_responseparameter has been consistently added to all email action configuration types, including the key vault implementation.
30476-30476: LGTM: Consistent spacing improves test readability.The addition of blank lines improves the readability of the test case by clearly separating the setup, action, and assertion phases.
Also applies to: 30484-30484
2 participants
Summary by CodeRabbit