Mail channel filter and stop #1649
Conversation
WalkthroughThe changes introduce a new endpoint in the FastAPI application for stopping mail reading associated with a bot, alongside a new static 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: 6
🧹 Outside diff range and nitpick comments (7)
kairon/shared/channels/mail/processor.py (2)
235-285: Ensure 'criteria' list is not empty before combining with 'AND'In the
generate_criteriamethod, if no criteria are added to thecriterialist, callingAND(*criteria)without any arguments may lead to unexpected behavior or errors. Consider adding a check to ensure that thecriterialist is not empty before combining, or handle the scenario when no criteria are specified.Apply this diff to handle empty criteria:
# Combine all criteria with AND + if criteria: return AND(*criteria) + else: + return None # or appropriate default criteriaEnsure that the calling code handles the case when
Noneor a default value is returned.
Line range hint
44-50: Use 'raise ... from e' to preserve exception contextIn the
get_mail_channel_state_datamethod, re-raising an exception without specifyingfrom eloses the original exception's context. To preserve the exception chain and aid in debugging, useraise AppException(str(e)) from e.Apply this diff to include the original exception context:
except Exception as e: - raise AppException(str(e)) + raise AppException(str(e)) from ekairon/shared/channels/mail/scheduler.py (1)
36-42: Consider using an appropriate HTTP method for state-changing operationsThe
request_stopmethod uses a GET request to stop email channel reading, which modifies the server state. According to RESTful principles, state-changing operations should not use GET requests. Consider using a POST or DELETE request to more accurately reflect the nature of the operation.Update the HTTP method and ensure corresponding changes are made in the server endpoint to handle the new method appropriately.
kairon/events/server.py (1)
155-158: Improve Path parameter definition and response message.
- Move the Path parameter definition outside the function to avoid potential issues with mutable defaults.
- Consider making the response message more descriptive.
+bot_path = Path(description="Bot id") + @app.get('/api/mail/stop/{bot}', response_model=Response) -def stop_mail_reading(bot: Text = Path(description="Bot id")): +def stop_mail_reading(bot: Text = bot_path): EventUtility.stop_channel_mail_reading(bot) - return {"data": None, "message": "Mail scheduler stopped!"} + return {"data": None, "message": f"Mail scheduler stopped for bot: {bot}"}🧰 Tools
🪛 Ruff (0.8.2)
156-156: Do not perform function call
Pathin 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 (1)
124-124: Remove unnecessary comment marker.The
#before the comment is redundant.-# # Test case when event_id is None + # Test case when event_id is Nonetests/unit_test/channels/mail_channel_test.py (2)
6-6: Remove unused importThe
ANDimport fromimap_toolsis not used in any of the test cases.-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)
279-324: Consider parameterizing test cases and cleanupThe test comprehensively covers various scenarios, but could benefit from the following improvements:
- Remove debug print statement on line 296
- Consider using
@pytest.mark.parametrizeto reduce code duplication- Add more descriptive assertion messages
Example of parameterized test:
@pytest.mark.parametrize("test_input,expected", [ ({"read_status": "seen"}, "((SEEN) (UID 124:*))"), ({"read_status": "unseen"}, "((UNSEEN) (UID 124:*))"), ({}, "((UID 124:*))"), ({"subjects": ["Test Subject", "another test subject"]}, '((OR SUBJECT "Test Subject" SUBJECT "another test subject") (UID 124:*))') ]) async def test_generate_criteria(self, mock_get_channel_config, test_input, expected): bot_id = pytest.mail_test_bot mock_get_channel_config.return_value = { 'config': { 'email_account': "[email protected]", 'email_password': "password", 'imap_server': "imap.testuser.com", } } mp = MailProcessor(bot=bot_id) mp.state.last_email_uid = 123 criteria = mp.generate_criteria(**test_input) assert criteria == expected, f"Failed for input: {test_input}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(2 hunks)kairon/shared/chat/processor.py(1 hunks)kairon/shared/utils.py(2 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)tests/unit_test/utility_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)
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/shared/channels/mail/processor.py
3-3: typing.List imported but unused
Remove unused import: typing.List
(F401)
15-15: kairon.shared.chat.data_objects.Channels imported but unused
Remove unused import: kairon.shared.chat.data_objects.Channels
(F401)
tests/unit_test/channels/mail_channel_test.py
6-6: imap_tools.AND imported but unused
Remove unused import: imap_tools.AND
(F401)
🔇 Additional comments (5)
kairon/shared/channels/mail/processor.py (1)
44-44: Addition of type annotation is approved
Adding a type annotation to the bot parameter in get_mail_channel_state_data enhances code clarity and facilitates type checking.
kairon/shared/chat/processor.py (1)
73-77: LGTM! Good error handling implementation.
The try-except block properly handles potential failures when stopping the mail scheduler without affecting the main channel deletion flow. The error logging is descriptive and includes relevant context.
kairon/shared/utils.py (1)
2226-2233: LGTM! Well-implemented string to list conversion
The implementation is robust and handles all edge cases properly:
- Empty strings and None values
- Whitespace trimming
- Empty elements removal
- Special characters and numeric values
tests/unit_test/utility_test.py (1)
3311-3335: LGTM! Comprehensive test coverage
The test cases thoroughly verify the string_to_list functionality:
- Edge cases (empty string, None, only commas)
- Special inputs (special chars, numbers)
- Common cases (single/multiple elements, whitespace)
tests/unit_test/channels/mail_channel_test.py (1)
588-591: LGTM!
The cleanup of Channels objects in the test teardown is properly implemented to prevent test data leakage.
| from kairon.exceptions import AppException | ||
| from kairon.shared.account.data_objects import Bot | ||
| from kairon.shared.channels.mail.constants import MailConstants | ||
| from kairon.shared.channels.mail.data_objects import MailResponseLog, MailStatus, MailChannelStateData | ||
| from kairon.shared.chat.data_objects import Channels |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import 'Channels'
The Channels class from kairon.shared.chat.data_objects is imported but not used. Please remove the unused import.
Apply this diff to remove the unused import:
- from kairon.shared.chat.data_objects import Channels🧰 Tools
🪛 Ruff (0.8.2)
15-15: kairon.shared.chat.data_objects.Channels imported but unused
Remove unused import: kairon.shared.chat.data_objects.Channels
(F401)
| @@ -1,14 +1,18 @@ | |||
| import asyncio | |||
| import time | |||
| from typing import List | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import 'List'
The List module from typing is imported but not used in the code. Consider removing it to clean up the imports.
Apply this diff to remove the unused import:
- from typing import List🧰 Tools
🪛 Ruff (0.8.2)
3-3: typing.List imported but unused
Remove unused import: typing.List
(F401)
| 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 'err_msg' parameter
In line 41, the err_msg parameter passed to Utility.execute_http_request is "Failed to request epoch", which is misleading in the context of stopping email channel reading. Please update the error message to accurately reflect the operation being performed.
Apply this diff to correct the error message:
err_msg="Failed to request epoch",
+ err_msg="Failed to stop email channel reading",Committable suggestion skipped: line range outside the PR's diff.
| - subjects | ||
| - ignore_subjects | ||
| - from_emails | ||
| - ignore_from_emails | ||
| - seen_status |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent naming of 'from' address fields
In the mail integration configuration, the optional fields from_emails and ignore_from_emails are added. In the code, these fields are referred to as from_addresses and ignore_from. To maintain consistency and avoid confusion, consider using the same naming convention in both the configuration and the code.
Optionally, apply this diff to update the field names:
optional_fields:
- interval
- intent
- subjects
- ignore_subjects
- - from_emails
- - ignore_from_emails
+ - from_addresses
+ - ignore_from
- seen_statusEnsure that the code and documentation are updated accordingly to reflect these changes.
📝 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.
| - subjects | |
| - ignore_subjects | |
| - from_emails | |
| - ignore_from_emails | |
| - seen_status | |
| - subjects | |
| - ignore_subjects | |
| - from_addresses | |
| - ignore_from | |
| - seen_status |
| mail_processor.update_event_id(None) | ||
| KScheduler().delete_job(event_id) | ||
| except Exception as e: | ||
| raise AppException(f"Failed to stop mail reading for bot {bot}. Error: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use 'raise ... from e' to preserve exception context
In line 74, within the except block of stop_channel_mail_reading, re-raising an exception without specifying from e loses the original exception's context. Including from e preserves the exception chain and aids in debugging.
Apply this diff to include the original exception context:
except Exception as e:
- raise AppException(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 e📝 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.
| raise AppException(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 e |
🧰 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)
| try: | ||
| MailScheduler.request_stop(bot) | ||
| except AppException: | ||
| pytest.fail("request_epoch() raised AppException unexpectedly!") |
There was a problem hiding this comment.
Fix incorrect error message in test assertion.
The error message references request_epoch() instead of request_stop().
try:
MailScheduler.request_stop(bot)
except AppException:
- pytest.fail("request_epoch() raised AppException unexpectedly!")
+ pytest.fail("request_stop() raised AppException unexpectedly!")📝 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.
| try: | |
| MailScheduler.request_stop(bot) | |
| except AppException: | |
| pytest.fail("request_epoch() raised AppException unexpectedly!") | |
| try: | |
| MailScheduler.request_stop(bot) | |
| except AppException: | |
| pytest.fail("request_stop() raised AppException unexpectedly!") |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
kairon/shared/channels/mail/processor.py (1)
233-282: Add input parameter validationWhile the implementation is solid, consider adding validation for the input parameters to ensure they are in the expected format and handle edge cases gracefully.
Add validation at the beginning of the method:
def generate_criteria(self, subjects=None, ignore_subjects=None, from_addresses=None, ignore_from=None, read_status="all"): + if read_status and not isinstance(read_status, str): + raise ValueError("read_status must be a string") + if read_status and read_status.lower() not in ["all", "seen", "unseen"]: + raise ValueError("read_status must be one of: all, seen, unseen") + + def validate_list_param(param, name): + if param and not isinstance(param, (list, tuple)): + raise ValueError(f"{name} must be a list or tuple") + + validate_list_param(subjects, "subjects") + validate_list_param(ignore_subjects, "ignore_subjects") + validate_list_param(from_addresses, "from_addresses") + validate_list_param(ignore_from, "ignore_from")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kairon/shared/channels/mail/processor.py(4 hunks)
🔇 Additional comments (2)
kairon/shared/channels/mail/processor.py (2)
233-282: Well-structured implementation of IMAP criteria generation
The implementation effectively combines multiple filtering criteria using proper IMAP constructs. The code is well-organized and handles both time-based and UID-based filtering appropriately.
319-320: Consider implications of mark_seen=False
Setting mark_seen=False means emails will remain unread after processing. This could lead to processing the same emails repeatedly if the last_processed_uid state is lost or reset.
Consider the following verification:
Consider adding a configuration option to control this behavior:
+ mark_as_read = mp.config.get('mark_emails_as_read', True)
- for msg in mp.mailbox.fetch(criteria, mark_seen=False):
+ for msg in mp.mailbox.fetch(criteria, mark_seen=mark_as_read):| subject = mp.config.get('subjects', "") | ||
| subject = Utility.string_to_list(subject) | ||
| ignore_subject = mp.config.get('ignore_subjects', "") | ||
| ignore_subject = Utility.string_to_list(ignore_subject) | ||
| from_addresses = mp.config.get('from_emails', "") | ||
| from_addresses = Utility.string_to_list(from_addresses) | ||
| ignore_from = mp.config.get('ignore_from_emails', "") | ||
| ignore_from = Utility.string_to_list(ignore_from) | ||
| read_status = mp.config.get('seen_status', 'all') | ||
| criteria = mp.generate_criteria(subject, ignore_subject, from_addresses, ignore_from, read_status) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for malformed configuration values
The configuration retrieval could fail if the values are malformed. Consider adding proper error handling.
Add error handling for configuration parsing:
- subject = mp.config.get('subjects', "")
- subject = Utility.string_to_list(subject)
+ try:
+ subject = mp.config.get('subjects', "")
+ subject = Utility.string_to_list(subject)
+ except Exception as e:
+ logger.error(f"Error parsing subjects configuration: {str(e)}")
+ subject = []Apply similar error handling for other configuration values.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Tests