mail read schedule stop mechanism, filters and their related tests #1645
Conversation
WalkthroughThe changes introduced in this pull request include the addition of a new endpoint in the FastAPI application for stopping mail reading associated with a bot. This is implemented through a new method in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
kairon/shared/channels/mail/processor.py (1)
46-48: Simplify the return statement indoes_mail_channel_existmethodYou can return the condition directly to make the code more concise.
Apply this diff:
def does_mail_channel_exist(bot: str): - if Channels.objects(bot=bot, connector_type=ChannelTypes.MAIL.value).first(): - return True - return False + return bool(Channels.objects(bot=bot, connector_type=ChannelTypes.MAIL.value).first())🧰 Tools
🪛 Ruff (0.8.2)
46-48: Return the condition directly
Inline condition
(SIM103)
tests/unit_test/channels/mail_channel_test.py (3)
6-6: Remove unused importThe
ANDimport fromimap_toolsis not used in the code.-from imap_tools import MailMessage, AND +from imap_tools import MailMessage🧰 Tools
🪛 Ruff (0.8.2)
6-6:
imap_tools.ANDimported but unusedRemove unused import:
imap_tools.AND(F401)
282-323: Enhance test coverage and remove debug printThe test method covers basic criteria generation well, but could be improved:
- Remove the debug print statement on line 296
- Add test cases for edge cases:
- Empty lists for subjects and from_addresses
- Invalid read_status value
- Special characters in subjects and from_addresses
- print(criteria) assert criteria == '((SEEN) (UID 124:*))'
617-626: Consider adding error scenario testsThe mail channel existence tests cover the basic cases well. Consider adding tests for error scenarios:
- Database connection errors
- Invalid bot IDs
- Malformed channel configurations
Example test case:
@patch('kairon.shared.channels.mail.processor.Channels') def test_mail_channel_exist_error(self, mock_channels): mock_channels.objects.side_effect = Exception("Database error") with pytest.raises(AppException): MailProcessor.does_mail_channel_exist("test_bot")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
kairon/events/server.py(1 hunks)kairon/events/utility.py(2 hunks)kairon/shared/channels/mail/processor.py(4 hunks)kairon/shared/channels/mail/scheduler.py(1 hunks)kairon/shared/chat/processor.py(1 hunks)metadata/integrations.yml(1 hunks)tests/unit_test/channels/mail_channel_test.py(3 hunks)tests/unit_test/channels/mail_scheduler_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/events/server.py
156-156: Do not perform function call Path in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
kairon/events/utility.py
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/unit_test/channels/mail_channel_test.py
6-6: imap_tools.AND imported but unused
Remove unused import: imap_tools.AND
(F401)
tests/unit_test/channels/mail_scheduler_test.py
145-145: Redefinition of unused test_request_stop_success from line 132
(F811)
kairon/shared/channels/mail/processor.py
46-48: Return the condition directly
Inline condition
(SIM103)
🔇 Additional comments (8)
kairon/shared/channels/mail/processor.py (2)
327-336: Handle missing or malformed configuration values gracefully
When retrieving configuration values like subjects, from_emails, etc., ensure that the code handles cases where these configurations might be missing or not in the expected format. This will prevent potential runtime errors.
Run the following script to verify the usage of configuration values:
281-283: Ensure correct handling of last_read_timestamp
The variable last_read_timestamp is calculated but not adjusted for time zones or potential daylight saving time changes. Consider using timezone-aware datetime objects to avoid potential issues with date and time calculations.
Run the following script to check for timezone-aware datetime usage:
metadata/integrations.yml (1)
88-92: Fields added to enhance mail integration configurability
The addition of subjects, ignore_subjects, from_emails, ignore_from_emails, and seen_status fields allows for more granular control over email processing.
kairon/events/server.py (1)
155-158: LGTM! The endpoint implementation looks good.
The implementation follows the established patterns in the codebase and properly handles the response.
🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not perform function call Path in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
tests/unit_test/channels/mail_scheduler_test.py (2)
111-127: LGTM! Test coverage for stop_channel_mail_reading is comprehensive
The test properly verifies the deletion of the job when stopping mail reading.
129-176: LGTM! Test coverage for request_stop scenarios is thorough
The tests cover all important scenarios:
- Success case
- Response not success case
- No channel exists case
🧰 Tools
🪛 Ruff (0.8.2)
145-145: Redefinition of unused test_request_stop_success from line 132
(F811)
kairon/shared/chat/processor.py (1)
73-78: LGTM! Mail scheduler stopping is properly handled
The implementation:
- Correctly checks for non-Slack connector types
- Properly handles exceptions with logging
- Maintains the existing functionality
tests/unit_test/channels/mail_channel_test.py (1)
591-615: LGTM! Well-structured test cases
The test method is comprehensive and covers various edge cases including:
- Empty elements and strings
- None values
- Special characters
- Numeric values
- Spaces and formatting
| from pydantic.validators import datetime | ||
| from imap_tools import MailBox, AND | ||
| from imap_tools import MailBox, AND, OR, NOT |
There was a problem hiding this comment.
Fix incorrect imports of datetime and timedelta
The datetime and timedelta classes are being imported from pydantic, which is incorrect and will lead to errors. They should be imported from the built-in datetime module.
Apply this diff to correct the imports:
-from pydantic.schema import timedelta
-from pydantic.validators import datetime
+from datetime import datetime, timedeltaCommittable suggestion skipped: line range outside the PR's diff.
| resp = Utility.execute_http_request( | ||
| "GET", | ||
| urljoin( | ||
| event_server_url, | ||
| f"/api/mail/stop/{bot}", | ||
| ), | ||
| err_msg="Failed to request epoch", | ||
| ) |
There was a problem hiding this comment.
Correct the error message in execute_http_request
The err_msg passed to Utility.execute_http_request should accurately reflect the operation being performed. Currently, it incorrectly states "Failed to request epoch" when the operation is to stop the mail channel.
Apply this diff to correct the error message:
resp = Utility.execute_http_request(
"GET",
urljoin(
event_server_url,
f"/api/mail/stop/{bot}",
),
- err_msg="Failed to request epoch",
+ err_msg="Failed to stop mail channel",
)📝 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.
| resp = Utility.execute_http_request( | |
| "GET", | |
| urljoin( | |
| event_server_url, | |
| f"/api/mail/stop/{bot}", | |
| ), | |
| err_msg="Failed to request epoch", | |
| ) | |
| resp = Utility.execute_http_request( | |
| "GET", | |
| urljoin( | |
| event_server_url, | |
| f"/api/mail/stop/{bot}", | |
| ), | |
| err_msg="Failed to stop mail channel", | |
| ) |
kairon/events/utility.py
Outdated
| except Exception as e: | ||
| raise logger.error(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}") |
There was a problem hiding this comment.
Fix incorrect exception handling in stop_channel_mail_reading
Raising logger.error() is not appropriate. You should log the error and then raise the exception to propagate it correctly.
Apply this diff to fix the exception handling:
except Exception as e:
- raise logger.error(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}")
+ logger.error(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}")
+ raise eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| def stop_mail_reading(bot: Text = Path(description="Bot id")): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid function call in parameter defaults
The Path function call in the parameter default could lead to runtime issues. Instead, declare the parameter type annotation separately.
-def stop_mail_reading(bot: Text = Path(description="Bot id")):
+def stop_mail_reading(
+ bot: Text = ..., # type: ignore
+) -> JSONResponse:
+ """Stop mail reading for a bot.
+
+ Args:
+ bot (Text): Bot id
+ """
+ bot = Path(description="Bot id")(bot)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not perform function call Path in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| @patch('kairon.shared.channels.mail.processor.MailProcessor.does_mail_channel_exist') | ||
| @patch('kairon.shared.channels.mail.scheduler.Utility.get_event_server_url') | ||
| @patch('kairon.shared.channels.mail.scheduler.Utility.execute_http_request') | ||
| def test_request_stop_success(mock_execute_http_request, mock_get_event_server_url, mock_imp): | ||
| mock_get_event_server_url.return_value = "http://localhost" | ||
| mock_execute_http_request.return_value = {'success': True} | ||
| mock_imp.return_value = True | ||
| bot = "test_bot" | ||
| try: | ||
| MailScheduler.request_stop(bot) | ||
| except AppException: | ||
| pytest.fail("request_epoch() raised AppException unexpectedly!") | ||
|
|
There was a problem hiding this comment.
Remove duplicate test function
The test function test_request_stop_success is defined twice. Remove the duplicate implementation.
-@patch('kairon.shared.channels.mail.processor.MailProcessor.does_mail_channel_exist')
-@patch('kairon.shared.channels.mail.scheduler.Utility.get_event_server_url')
-@patch('kairon.shared.channels.mail.scheduler.Utility.execute_http_request')
-def test_request_stop_success(mock_execute_http_request, mock_get_event_server_url, mock_imp):
- mock_get_event_server_url.return_value = "http://localhost"
- mock_execute_http_request.return_value = {'success': True}
- mock_imp.return_value = True
- bot = "test_bot"
- try:
- MailScheduler.request_stop(bot)
- except AppException:
- pytest.fail("request_epoch() raised AppException unexpectedly!")🧰 Tools
🪛 Ruff (0.8.2)
145-145: Redefinition of unused test_request_stop_success from line 132
(F811)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
kairon/events/utility.py (1)
69-72: Consider handling the no-op case explicitly.The method silently succeeds when no event_id exists. Consider either:
- Logging this case for better observability
- Throwing an exception if there's no active mail reading to stop
- Returning a status indicating whether anything was actually stopped
Here's one possible implementation:
mail_processor = MailProcessor(bot) event_id = mail_processor.state.event_id if event_id: mail_processor.update_event_id(None) KScheduler().delete_job(event_id) + logger.info(f"Successfully stopped mail reading for bot {bot}") + else: + logger.info(f"No active mail reading found for bot {bot}")tests/unit_test/channels/mail_scheduler_test.py (5)
124-124: Fix comment formatting.Remove the extra '#' and fix the indentation.
-# # Test case when event_id is None + # Test case when event_id is None
111-127: Add test case for non-existent event_id.The test only covers the case where event_id exists. Consider adding a test case where
event_idis None to ensure complete coverage of the stop functionality.
140-140: Fix incorrect error message in pytest.fail.The error message refers to "request_epoch" but should refer to "request_stop".
- pytest.fail("request_epoch() raised AppException unexpectedly!") + pytest.fail("request_stop() raised AppException unexpectedly!")
146-146: Fix inconsistent test function naming.The function name uses double underscore which is inconsistent with other test names in the file.
-def test_request_stop__response_not_success +def test_request_stop_response_not_success
111-164: Add error handling test for stop_channel_mail_reading.Consider adding a test case that verifies error handling in
stop_channel_mail_reading, similar totest_schedule_channel_mail_reading_exception. This would ensure consistent error handling coverage across the codebase.Example test case:
@patch('kairon.events.utility.KScheduler.delete_job') @patch('kairon.events.utility.KScheduler.__init__', return_value=None) @patch('kairon.shared.channels.mail.processor.MailProcessor') @patch('pymongo.MongoClient', autospec=True) def test_stop_channel_mail_reading_exception(mock_mongo, mock_mail_processor, mock_kscheduler, mock_delete_job): from kairon.events.utility import EventUtility bot = "test_bot" mock_mail_processor.side_effect = Exception("Test Exception") with pytest.raises(AppException) as excinfo: EventUtility.stop_channel_mail_reading(bot) assert str(excinfo.value) == f"Failed to stop mail reading for bot {bot}. Error: Test Exception"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/events/utility.py(2 hunks)tests/unit_test/channels/mail_scheduler_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/events/utility.py
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
kairon/events/utility.py (2)
8-8: LGTM! Good cleanup of imports.
Removing unused imports helps maintain code cleanliness.
63-74: 🛠️ Refactor suggestion
Improve exception handling and add validation.
The implementation needs the following improvements:
- Use
raise ... from errpattern for proper exception chaining - Add input validation for the bot parameter
- Add success logging for better observability
Apply this diff to implement the suggested improvements:
@staticmethod
def stop_channel_mail_reading(bot: str):
from kairon.shared.channels.mail.processor import MailProcessor
+ if not bot:
+ raise AppException("Bot parameter cannot be empty")
+
try:
mail_processor = MailProcessor(bot)
event_id = mail_processor.state.event_id
if event_id:
mail_processor.update_event_id(None)
KScheduler().delete_job(event_id)
+ logger.info(f"Successfully stopped mail reading for bot {bot}")
except Exception as e:
- raise AppException(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}")
+ logger.error(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}")
+ raise AppException(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/unit_test/channels/mail_scheduler_test.py (2)
132-141: Remove duplicate test function.
The test function test_request_stop_success is defined twice.
154-164: LGTM!
The test properly validates the edge case where the mail channel doesn't exist.
| @@ -70,6 +70,12 @@ def delete_channel_config(bot: Text, **kwargs): | |||
| :return: None | |||
| """ | |||
| kwargs.update({"bot": bot}) | |||
| if kwargs.get("connector_type") != ChannelTypes.SLACK.value: | |||
| @staticmethod | ||
| def does_mail_channel_exist(bot:str): | ||
| """ | ||
| Check if mail channel exists |
There was a problem hiding this comment.
use existing function Utility.is_exist()
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests