Mail channel filter #1651
Mail channel filter #1651
Conversation
WalkthroughThe changes introduce a new endpoint in the FastAPI application for stopping mail reading associated with a bot. This includes the addition of a 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: 0
🧹 Outside diff range and nitpick comments (10)
kairon/shared/channels/mail/processor.py (1)
310-317: Inconsistent Variable Naming: Use Plural Form for ListsVariables holding lists should be named in the plural form for clarity. Recommend renaming
subjecttosubjects,ignore_subjecttoignore_subjects, andignore_fromtoignore_from_addressesfor consistency withfrom_addresses.Apply this diff to improve variable naming consistency:
- 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) - ignore_from = mp.config.get('ignore_from_emails', "") - ignore_from = Utility.string_to_list(ignore_from) + ignore_from_addresses = mp.config.get('ignore_from_emails', "") + ignore_from_addresses = Utility.string_to_list(ignore_from_addresses)kairon/shared/channels/mail/scheduler.py (1)
34-46: Handle Non-Existent Mail Channel GracefullyIn the
request_stopmethod, when the mail channel does not exist, anAppExceptionis raised. Consider handling this scenario more gracefully by logging a warning instead of raising an exception, allowing the method to exit without error if the channel is already stopped.Apply this diff to adjust the exception handling:
else: - raise AppException("Mail channel does not exist") + logger.warning("Mail channel does not exist for bot: %s", bot)metadata/integrations.yml (1)
88-92: Consider Providing Default Values or Validation for New FieldsTo enhance robustness, consider providing default values or input validation for the new optional fields to handle cases where users may provide incorrect configurations.
For example, ensure that
seen_statusaccepts only valid options like'all','seen', or'unseen'.kairon/events/utility.py (1)
73-74: Chain Exceptions for Clearer Error TracebackWhen raising an exception inside an
exceptblock, it's best practice to useraise ... from eto maintain the original traceback, aiding in debugging.Apply this diff to chain the exception:
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🧰 Tools
🪛 Ruff (0.8.2)
74-74: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
kairon/events/server.py (1)
155-158: Consider adding error response documentation.The endpoint implementation looks good, but consider documenting possible error responses in the FastAPI path operation decorator.
-@app.get('/api/mail/stop/{bot}', response_model=Response) +@app.get( + '/api/mail/stop/{bot}', + response_model=Response, + responses={ + 404: {"description": "Mail channel not found"}, + 500: {"description": "Internal server error while stopping mail scheduler"} + } +)🧰 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)
111-126: Add test case for when event_id is None.The test only covers the case when event_id exists. Consider adding a test case for when event_id is None to ensure complete coverage.
# Add this test case mock_mail_processor_instance.state.event_id = None EventUtility.stop_channel_mail_reading(bot) mock_delete_job.assert_not_called()kairon/shared/chat/processor.py (1)
73-77: Consider selective exception handling.The current implementation catches all exceptions. Consider catching specific exceptions that might occur during the mail scheduler stop operation.
try: from kairon.shared.channels.mail.scheduler import MailScheduler MailScheduler.request_stop(bot) -except Exception as e: +except (ConnectionError, AppException) as e: logger.error(f"Error while stopping mail scheduler for bot {bot}. Error: {str(e)}")tests/unit_test/channels/mail_channel_test.py (2)
280-323: Consider parameterizing test cases and adding negative scenarios.The test provides good coverage of various email filtering criteria combinations. However, consider these improvements:
- Parameterize test cases using
@pytest.mark.parametrizeto improve maintainability- Add negative test cases (e.g., invalid read_status, empty lists)
Example parameterization:
@pytest.mark.parametrize("test_input,expected", [ ({"read_status": "seen"}, "((SEEN) (UID 124:*))"), ({"read_status": "unseen"}, "((UNSEEN) (UID 124:*))"), ({"subjects": ["Test Subject"]}, '((OR SUBJECT "Test Subject") (UID 124:*))'), # Add more test cases... ]) async def test_generate_criteria(self, test_input, expected, mock_get_channel_config): mp = MailProcessor(bot=pytest.mail_test_bot) mp.state.last_email_uid = 123 criteria = mp.generate_criteria(**test_input) assert criteria == expected
588-591: Clean up extra newlines at the end of the file.Maintain a single newline at the end of the file instead of multiple blank lines.
Channels.objects(connector_type=ChannelTypes.MAIL.value).delete() - - - +kairon/shared/utils.py (1)
2226-2234: LGTM! Consider enhancing the docstring.The implementation is clean and handles edge cases well. Consider adding parameter descriptions and return type to the docstring for better documentation.
@staticmethod def string_to_list(comma_sep_string: str, delimilter: str = ",") -> List[str]: """ - Convert comma separated string to list + Convert a string to list by splitting on delimiter. + + Args: + comma_sep_string: Input string to split + delimilter: Character to split on, defaults to comma + + Returns: + List[str]: List of non-empty strings after splitting and stripping whitespace """ if not comma_sep_string: return [] return [item.strip() for item in comma_sep_string.split(delimilter) if item.strip()]
📜 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)
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 (10)
kairon/shared/channels/mail/processor.py (3)
42-42: Type Hint Added to get_mail_channel_state_data
Adding the type annotation bot: str enhances code clarity and facilitates static type checking.
233-283: New generate_criteria Method Implemented Correctly
The generate_criteria method effectively constructs IMAP criteria based on provided parameters, improving the flexibility and specificity of email fetching.
269-278: Ensure Correct Handling of Initial Email Fetch
When last_processed_uid is 0, the base criteria uses date_gte with the current date minus the interval. Verify that this approach correctly fetches emails from the intended timeframe and that time zone differences do not cause any discrepancies.
kairon/shared/channels/mail/scheduler.py (1)
31-47: New request_stop Method Added
The request_stop method correctly implements the functionality to stop email channel reading for a bot. It checks for the existence of the mail channel and handles HTTP requests appropriately.
metadata/integrations.yml (1)
88-92: New Optional Fields Added to Mail Integration
The addition of subjects, ignore_subjects, from_emails, ignore_from_emails, and seen_status expands the configurability of the mail integration, allowing for finer-grained email filtering.
kairon/events/utility.py (1)
62-74: New Method stop_channel_mail_reading Implemented
The stop_channel_mail_reading method correctly stops the mail reading process by updating the event ID and deleting the scheduled job.
🧰 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)
kairon/events/server.py (1)
152-153: LGTM!
The formatting change maintains consistency with the rest of the file.
tests/unit_test/channels/mail_scheduler_test.py (1)
128-162: LGTM! Comprehensive test coverage.
The test cases effectively cover:
- Successful stop request
- Failed response handling
- Non-existent channel scenario
tests/unit_test/channels/mail_channel_test.py (1)
6-6: LGTM!
While AND is not directly used, it provides context about the underlying implementation being tested in the test_generate_criteria method.
🧰 Tools
🪛 Ruff (0.8.2)
6-6: imap_tools.AND imported but unused
Remove unused import: imap_tools.AND
(F401)
tests/unit_test/utility_test.py (1)
3311-3335: LGTM! Comprehensive test coverage.
The test cases thoroughly validate the functionality including edge cases and various input scenarios. The assertions are clear and verify the expected behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation