Mail channel filter and stop #1650
Conversation
WalkthroughThe pull request introduces several enhancements to the mail handling functionality within the FastAPI application. A new endpoint, Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (9)
kairon/shared/channels/mail/processor.py (2)
233-283: Consider adding type annotations to function parameters for better clarityAdding type annotations to function parameters enhances code readability and aids in static type checking.
Apply this diff to add type hints:
-def generate_criteria(self, subjects=None, ignore_subjects=None, from_addresses=None, ignore_from=None, - read_status="all"): +def generate_criteria( + self, + subjects: Optional[List[str]] = None, + ignore_subjects: Optional[List[str]] = None, + from_addresses: Optional[List[str]] = None, + ignore_from: Optional[List[str]] = None, + read_status: str = "all", +) -> AND:
310-317: Rename variables to reflect that they are listsFor better code readability, consider renaming
subjecttosubjectsandignore_subjecttoignore_subjects, as they represent lists.Apply this diff to rename the variables:
- 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) + subjects = mp.config.get('subjects', "") + subjects = Utility.string_to_list(subjects) + ignore_subjects = mp.config.get('ignore_subjects', "") + ignore_subjects = Utility.string_to_list(ignore_subjects)kairon/shared/channels/mail/scheduler.py (1)
41-41: Correct the error message to reflect the operationThe
err_msgparameter currently says "Failed to request epoch", which is misleading. Update it 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",kairon/events/server.py (1)
155-158: Move Path call out of parameter defaultsTo address the B008 warning, consider moving the Path call within the function body:
-def stop_mail_reading(bot: Text = Path(description="Bot id")): +def stop_mail_reading(bot: Text): + """Stop mail reading for the specified bot.""" + bot = Path(description="Bot id")(bot) EventUtility.stop_channel_mail_reading(bot) return {"data": None, "message": "Mail scheduler stopped!"}🧰 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)
128-139: Fix test name to match its purposeThe error message mentions "request_epoch" but the test is for "request_stop".
- pytest.fail("request_epoch() raised AppException unexpectedly!") + pytest.fail("request_stop() raised AppException unexpectedly!")tests/unit_test/channels/mail_channel_test.py (4)
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)
296-296: Remove debug print statementDebug print statements should not be committed to the codebase.
- print(criteria)
282-323: Consider splitting test cases and adding edge casesWhile the test coverage is good for the happy path, consider:
- Splitting into separate test methods for better readability (e.g.,
test_generate_criteria_seen,test_generate_criteria_subjects, etc.)- Adding edge cases:
- Empty lists for subjects and from_addresses
- None values for parameters
- Special characters in subjects and email addresses
Example structure:
@pytest.mark.asyncio async def test_generate_criteria_seen(self, mock_get_channel_config): mp = MailProcessor(bot=self.bot_id) mp.state.last_email_uid = 123 criteria = mp.generate_criteria(read_status="seen") assert criteria == '((SEEN) (UID 124:*))' @pytest.mark.asyncio async def test_generate_criteria_empty_lists(self, mock_get_channel_config): mp = MailProcessor(bot=self.bot_id) mp.state.last_email_uid = 123 criteria = mp.generate_criteria(subjects=[], from_addresses=[]) assert criteria == '((UID 124:*))'
589-591: Remove extra blank lines at the end of fileKeep only one blank line at the end of the file.
Channels.objects(connector_type=ChannelTypes.MAIL.value).delete() - -
📜 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/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)
🔇 Additional comments (7)
metadata/integrations.yml (1)
88-92: New optional fields added correctly to the mail integration
The addition of subjects, ignore_subjects, from_emails, ignore_from_emails, and seen_status enhances configurability.
kairon/events/server.py (1)
152-158: LGTM! The implementation looks clean and consistent.
The new endpoint follows the established patterns and properly integrates with the existing codebase.
🧰 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-126: LGTM! Good test coverage for stop functionality.
The test properly verifies that delete_job is called when stopping mail reading.
141-163: LGTM! Comprehensive error case coverage.
The tests properly verify behavior for:
- Response not successful
- Non-existent channel
kairon/shared/chat/processor.py (1)
73-77: LGTM! Robust error handling implementation.
The error handling ensures that channel deletion proceeds even if stopping the mail scheduler fails. The error is properly logged for debugging purposes.
kairon/shared/utils.py (1)
2226-2233: LGTM! Clean implementation of string_to_list method.
The implementation:
- Handles edge cases (empty strings, None)
- Uses efficient list comprehension
- Has good input validation
- Is well tested
tests/unit_test/utility_test.py (1)
3311-3334: LGTM! Comprehensive test coverage for string_to_list method.
The test cases cover:
- Edge cases (empty elements, only commas, empty string, None)
- Special characters and numeric values
- Spaces handling
- Single element inputs
| resp = Utility.execute_http_request( | ||
| "GET", | ||
| urljoin( | ||
| event_server_url, | ||
| f"/api/mail/stop/{bot}", | ||
| ), |
There was a problem hiding this comment.
Use appropriate HTTP method for stopping the email channel
Using GET for an operation that changes the server state is not recommended. Consider using POST to adhere to RESTful best practices.
Apply this diff to change the HTTP method:
- resp = Utility.execute_http_request(
- "GET",
+ resp = Utility.execute_http_request(
+ "POST",
urljoin(
event_server_url,
f"/api/mail/stop/{bot}",
),
err_msg="Failed to request epoch",
)📝 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}", | |
| ), | |
| resp = Utility.execute_http_request( | |
| "POST", | |
| urljoin( | |
| event_server_url, | |
| f"/api/mail/stop/{bot}", | |
| ), |
| 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 exception chaining to preserve the original traceback
In the except block, raise the new exception using from e to maintain the original traceback, aiding in debugging.
Apply this diff to chain exceptions:
- 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)
Summary by CodeRabbit
New Features
Bug Fixes
Tests