Mail channel first #1617
Mail channel first #1617
Conversation
WalkthroughThis pull request introduces several enhancements to the Kairon framework, primarily focusing on mail channel processing. Key changes include the addition of new command-line interface subcommands for mail processing and reading, along with new API endpoints for retrieving logs. New event classes and utility methods for handling mail events are implemented, and modifications to existing classes enhance mail processing capabilities. The configuration files are updated to support these new features, and extensive test coverage is added to ensure functionality and error handling. 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: 18
🧹 Outside diff range and nitpick comments (38)
tests/unit_test/data_processor/data_processor_test.py (2)
Line range hint
3988-4132: Optimize dictionary key checksConsider simplifying the dictionary key checks by removing the redundant
.keys()calls.Apply these changes for better readability and performance:
-assert domain.responses.keys().__len__() == 29 +assert len(domain.responses) == 29 -assert len([intent for intent in domain.intent_properties.keys() if +assert len([intent for intent in domain.intent_properties if🧰 Tools
🪛 Ruff (0.8.0)
3992-3992: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
3994-3994: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Line range hint
7133-7632: Consider reducing test code duplicationThere's significant duplication in domain verification logic across multiple test cases. Consider extracting common assertion patterns into helper methods.
Example helper method:
def assert_domain_configuration(domain, expected_slots, expected_entities, expected_intents, expected_actions): assert len(domain.slots) == expected_slots assert len(domain.entities) == expected_entities assert len(domain.intents) == expected_intents assert len(domain.user_actions) == expected_actionsThis would make the tests more maintainable and reduce the likelihood of errors when updating assertions.
🧰 Tools
🪛 Ruff (0.8.0)
7282-7282: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
7284-7284: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
kairon/events/definitions/mail_channel.py (3)
99-99: Remove unused variableis_initializedThe variable
is_initializedis assigned but never used. Removing it will clean up the code and eliminate unnecessary variables.Apply this diff to remove the unused variable:
- is_initialized = False🧰 Tools
🪛 Ruff (0.8.0)
99-99: Local variable
is_initializedis assigned to but never usedRemove assignment to unused variable
is_initialized(F841)
90-90: Remove debugThe
print(vals)statement appears to be a debug statement and should be removed from production code to avoid unintended console output.Apply this diff to remove the debug statement:
- print(vals)
26-26: Handle potential exceptions during connection validationThe
validate_smtp_connectionandvalidate_imap_connectionmethods might raise exceptions that are not handled within thevalidatemethods ofMailProcessEventandMailReadEvent. Consider adding exception handling to manage potential connection errors gracefully.Modify the
validatemethods to handle exceptions:def validate(self): """ Validate mail channel exists """ try: return MailProcessor.validate_smtp_connection(self.bot) except Exception as e: logger.error(f"SMTP validation failed: {str(e)}") raise AppException("SMTP validation failed") from eAlso applies to: 71-71
tests/unit_test/channels/mail_channel_test.py (4)
1-1: Remove unused importsThe imports
asyncioanduuid6.uuid7are not used in the code and should be removed to clean up the module.Apply this diff to remove the unused imports:
- import asyncio- from uuid6 import uuid7Also applies to: 9-9
🧰 Tools
🪛 Ruff (0.8.0)
1-1:
asyncioimported but unusedRemove unused import:
asyncio(F401)
14-14: Use capitalized environment variableSYSTEM_FILEEnvironment variable names should be uppercase to follow Python conventions and improve code readability. Replace
system_filewithSYSTEM_FILE.Apply this diff to correct the environment variable:
- os.environ["system_file"] = "./tests/testing_data/system.yaml" + os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Also, ensure that any references to this environment variable in the code are updated accordingly.
🧰 Tools
🪛 Ruff (0.8.0)
14-14: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
431-432: Simplify nestedwithstatements and remove unused variablemock_saveCombine nested
withstatements into a single line to improve readability. Additionally, the variablemock_saveis assigned but not used; consider removing it.Apply this diff to simplify and clean up the code:
- with patch.object(MailChannelStateData, 'objects') as mock_objects: - mock_objects.return_value.first.return_value = None - with patch.object(MailChannelStateData, 'save', return_value=None) as mock_save: - with patch('kairon.shared.channels.mail.data_objects.MailChannelStateData', return_value=mock_state): + with patch.object(MailChannelStateData, 'objects') as mock_objects, \ + patch('kairon.shared.channels.mail.data_objects.MailChannelStateData', return_value=mock_state): mock_objects.return_value.first.return_value = None result = MailProcessor.get_mail_channel_state_data(bot_id)🧰 Tools
🪛 Ruff (0.8.0)
431-432: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
431-431: Local variable
mock_saveis assigned to but never usedRemove assignment to unused variable
mock_save(F841)
431-431: Remove unused variablemock_saveThe variable
mock_saveis assigned but never used. Removing it will clean up the test code.Apply this diff to remove the unused variable:
- with patch.object(MailChannelStateData, 'save', return_value=None) as mock_save:🧰 Tools
🪛 Ruff (0.8.0)
431-432: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
431-431: Local variable
mock_saveis assigned to but never usedRemove assignment to unused variable
mock_save(F841)
tests/unit_test/utility_test.py (2)
Line range hint
2029-2032: Handle potential exceptions in email verification testsIn the
test_verify_email_enable_disposable_email, the test relies on external API responses. Ensure that the test cases handle unexpected API failures or changes gracefully, possibly by adding mock responses or exception handling.Consider using mocking frameworks to simulate API responses and avoid flaky tests.
Line range hint
2171-2174: Use proper assertions for password validation testsIn the
test_valid_passwordmethod, the exception message is being checked usingassert str(error.value) == err_msg. Consider usingassert str(error.value) == expected_error_messageto make the assertion clearer.Apply this diff to improve the assertion:
- assert str(error.value) == err_msg + assert str(error.value) == "Password length must be 8"kairon/shared/channels/mail/constants.py (3)
4-5: Consider making mail server configurations more flexible.The default SMTP and IMAP servers are hardcoded to Gmail's servers. Consider:
- Making these configurable through environment variables
- Adding support for other popular email providers
- Documenting the requirements for custom SMTP/IMAP servers
8-8: Make email template configurable.The default template has hardcoded branding. Consider:
- Making the template configurable through environment variables or configuration files
- Supporting HTML template files
- Adding placeholders for customizable branding
10-10: Document the rationale for batch size.Please add a comment explaining:
- Why 4 was chosen as the batch size
- Performance implications
- Whether this value should be configurable
kairon/shared/channels/mail/scheduler.py (1)
13-17: Consider parallel connection validation.The SMTP and IMAP connection validations are performed sequentially. Consider:
- Implementing parallel validation for better performance
- Adding connection timeouts
- Caching validation results briefly to prevent repeated checks
kairon/shared/channels/mail/data_objects.py (4)
4-7: Clean up unused importsThe following imports are not used in this file:
BooleanFieldfrommongoengineAppExceptionfromkairon.exceptionsauditlogandpush_notificationfromkairon.shared.data.signals-from mongoengine import Document, StringField, ListField, FloatField, BooleanField, DictField, IntField +from mongoengine import Document, StringField, ListField, FloatField, DictField, IntField -from kairon.exceptions import AppException -from kairon.shared.data.signals import auditlog, push_notification🧰 Tools
🪛 Ruff (0.8.0)
4-4:
mongoengine.BooleanFieldimported but unusedRemove unused import:
mongoengine.BooleanField(F401)
5-5:
kairon.exceptions.AppExceptionimported but unusedRemove unused import:
kairon.exceptions.AppException(F401)
7-7:
kairon.shared.data.signals.auditlogimported but unusedRemove unused import
(F401)
7-7:
kairon.shared.data.signals.push_notificationimported but unusedRemove unused import
(F401)
11-21: Add class documentationConsider adding a docstring to explain the purpose of this class and its fields, especially the significance of
last_email_uidin mail processing.class MailChannelStateData(Document): + """ + Maintains the state of mail channel processing for each bot. + + Fields: + event_id: Identifier for the current processing event + last_email_uid: Last processed email UID to prevent reprocessing + bot: Bot identifier + timestamp: Last update timestamp + """ event_id = StringField()
23-26: Standardize enum value casingThe enum values use inconsistent casing. Consider using all uppercase for enum values as per Python conventions.
class MailStatus(Enum): - Processing = "processing" + PROCESSING = "processing" SUCCESS = "success" FAILED = "failed"
40-40: Update default status to use new enum valueIf the enum value casing is standardized, update this line to use the new enum value.
- status = StringField(required=True, default=MailStatus.Processing.value) + status = StringField(required=True, default=MailStatus.PROCESSING.value)kairon/cli/mail_channel_process.py (1)
32-34: Add example in help text for mails argumentConsider adding an example JSON structure in the help text to guide users.
mail_parser.add_argument('mails', type=str, - help="json representing List of mails to be processed", + help='JSON array of mails to process. Example: \'[{"subject": "Test", "body": "Hello"}]\'', action='store')kairon/__init__.py (2)
47-48: Consider splitting long import statement for better readabilityThe import statement is becoming lengthy. Consider splitting it across multiple lines for better readability.
-from kairon.cli import importer, training, testing, conversations_deletion, translator, delete_logs,\ - message_broadcast,content_importer, mail_channel_process, mail_channel_read +from kairon.cli import ( + importer, + training, + testing, + conversations_deletion, + translator, + delete_logs, + message_broadcast, + content_importer, + mail_channel_process, + mail_channel_read, +)
Line range hint
1-46: Update CLI usage documentationThe docstring needs to be updated to include the new mail channel commands.
Add the following to the usage section:
""" CLI to train or import(and validate) data into kairon. Usage: ... Delete audit logs: kairon delete-logs + + Process mail channel: + kairon mail-channel-process <botid> <userid> + + Read mail channel: + kairon mail-channel-read <botid> <userid> """tests/unit_test/channels/mail_scheduler_test.py (2)
8-10: Use uppercase for environment variable nameFollow Python conventions by using uppercase for environment variables.
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.0)
8-8: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
56-69: Uncomment and complete the mailbox reading testThe commented test case appears to be important for testing mailbox reading functionality. Consider implementing it properly.
Would you like me to help implement this test case? I can provide a complete implementation that covers various scenarios including error cases.
kairon/chat/agent_processor.py (1)
82-99: Consider extracting common agent loading logicTo reduce code duplication between
reloadandget_agent_without_cache, consider extracting the common logic.+ @staticmethod + def _load_agent(bot: str, use_store: bool = True, cache_agent: bool = True) -> Agent: + """Internal method to load a bot agent with common logic.""" + endpoint = AgentProcessor.mongo_processor.get_endpoints( + bot, raise_exception=False + ) + action_endpoint = Utility.get_action_url(endpoint) + model_path = Utility.get_latest_model(bot) + domain = AgentProcessor.mongo_processor.load_domain(bot) + + if use_store: + mongo_store = Utility.get_local_mongo_store(bot, domain) + lock_store_endpoint = Utility.get_lock_store(bot) + agent = KaironAgent.load(model_path, action_endpoint=action_endpoint, + tracker_store=mongo_store, lock_store=lock_store_endpoint) + else: + agent = KaironAgent.load(model_path, action_endpoint=action_endpoint) + + agent.model_ver = model_path.split("/")[-1] + + if cache_agent: + bot_settings = AgentProcessor.mongo_processor.get_bot_settings(bot, "system") + AgentProcessor.cache_provider.set(bot, agent, is_billed=bot_settings.is_billed) + + return agent @staticmethod def get_agent_without_cache(bot: str, use_store: bool = True) -> Agent: """Load a bot agent without using cache.""" try: return AgentProcessor._load_agent(bot, use_store, cache_agent=False) except Exception as e: logging.exception(e) raise AppException("Failed to load bot agent!") from e @staticmethod def reload(bot: Text): """Reload bot agent.""" try: AgentProcessor._load_agent(bot, use_store=True, cache_agent=True) except Exception as e: logging.exception(e) raise AppException("Bot has not been trained yet!") from ekairon/shared/constants.py (1)
161-163: Consider adding additional mail-related system slotsThe current slots cover basic mail properties. Consider adding additional slots that might be useful for mail processing:
sender_emailrecipient_emailattachment_countattachment_namestests/unit_test/data_processor/agent_processor_test.py (2)
138-142: LGTM! Consider adding negative test casesThe test looks good but could be enhanced with additional test cases:
- Test with non-existent bot
- Test with invalid bot ID
- Test with different cache flag values
143-144: Remove unnecessary empty linesThese consecutive empty lines are not needed and should be removed for consistency with the rest of the file.
tests/integration_test/event_service_test.py (1)
536-545: Add test cases for error scenarios.The current test only covers the happy path. Consider adding the following test cases:
- Invalid bot ID
- Error during scheduling
- Missing bot parameter
Here's an example implementation:
@patch('kairon.events.utility.EventUtility.schedule_channel_mail_reading') def test_request_epoch_invalid_bot(mock_epoch): mock_epoch.side_effect = ValueError("Invalid bot") response = client.get('/api/mail/schedule/invalid_bot') assert response.status_code == 422 resp = response.json() assert not resp['success'] assert "Invalid bot" in resp['message'] def test_request_epoch_missing_bot(): response = client.get('/api/mail/schedule/') assert response.status_code == 404tests/unit_test/cli_test.py (3)
323-324: Use capitalized environment variable name.Following Python conventions, environment variables should be in uppercase.
- os.environ["system_file"] = "./tests/testing_data/system.yaml" + os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.0)
323-323: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
332-336: Combine nested with statements.Simplify the code by combining nested with statements.
- with patch('argparse.ArgumentParser.parse_args', - return_value=argparse.Namespace(func=process_channel_mails, - bot="test_bot", - user="test_user", mails=data)): - with pytest.raises(ValueError): - cli() + with ( + patch('argparse.ArgumentParser.parse_args', + return_value=argparse.Namespace(func=process_channel_mails, + bot="test_bot", + user="test_user", mails=data)), + pytest.raises(ValueError) + ): + cli()Also applies to: 345-349
319-361: Add test cases for missing required parameters.The test suite should verify behavior when required parameters are missing.
Add the following test cases:
def test_mail_channel_process_missing_bot(self): from kairon.cli.mail_channel_process import process_channel_mails with patch('argparse.ArgumentParser.parse_args', return_value=argparse.Namespace(func=process_channel_mails, user="test_user", mails="[]")): with pytest.raises(AttributeError) as e: cli() assert str(e).__contains__("'Namespace' object has no attribute 'bot'") def test_mail_channel_process_missing_user(self): from kairon.cli.mail_channel_process import process_channel_mails with patch('argparse.ArgumentParser.parse_args', return_value=argparse.Namespace(func=process_channel_mails, bot="test_bot", mails="[]")): with pytest.raises(AttributeError) as e: cli() assert str(e).__contains__("'Namespace' object has no attribute 'user'")🧰 Tools
🪛 Ruff (0.8.0)
323-323: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
345-349: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
kairon/shared/data/data_models.py (1)
1346-1351: Add field validation and documentation toMailConfigRequestclassConsider adding:
- Field validation to ensure non-empty values for required fields (
intent,classification_prompt)- Class docstring describing the purpose and usage of this configuration class
class MailConfigRequest(BaseModel): + """Configuration for mail channel processing. + + Attributes: + intent: Intent to be used for classification + entities: List of entities to extract + subjects: List of mail subjects to match + classification_prompt: Prompt used for mail classification + reply_template: Optional template for mail replies + """ intent: str entities: list[str] = [] subjects: list[str] = [] classification_prompt: str reply_template: str = None + + @validator("intent", "classification_prompt") + def validate_required_fields(cls, v): + from kairon.shared.utils import Utility + if Utility.check_empty_string(v): + raise ValueError(f"This field cannot be empty") + return vtests/unit_test/chat/chat_test.py (1)
942-966: Enhance test coverage fortest_process_messages_via_botWhile the happy path is well tested, consider adding:
- Error cases (e.g., when handle_message fails)
- Assertions for metadata handling
- Edge cases with empty/invalid messages
@pytest.mark.asyncio @patch("kairon.chat.utils.AgentProcessor.get_agent_without_cache") @patch("kairon.chat.utils.ChatUtils.get_metadata") async def test_process_messages_via_bot_error(mock_get_metadata, mock_get_agent_without_cache): messages = ["/greet"] account = 1 bot = "test_bot" user = "test_user" metadata = {"key": "value"} mock_get_metadata.return_value = metadata mock_model = MagicMock() mock_get_agent_without_cache.return_value = mock_model mock_model.handle_message = AsyncMock(side_effect=Exception("Error processing message")) with pytest.raises(Exception, match="Error processing message"): await ChatUtils.process_messages_via_bot(messages, account, bot, user, False, metadata) @pytest.mark.asyncio @patch("kairon.chat.utils.AgentProcessor.get_agent_without_cache") @patch("kairon.chat.utils.ChatUtils.get_metadata") async def test_process_messages_via_bot_empty_messages(mock_get_metadata, mock_get_agent_without_cache): messages = [] account = 1 bot = "test_bot" user = "test_user" responses = await ChatUtils.process_messages_via_bot(messages, account, bot, user, False) assert len(responses) == 0tests/unit_test/events/definitions_test.py (2)
1307-1322: Enhance assertions in mail read event testAdd more specific assertions to verify the behavior.
@patch('kairon.shared.channels.mail.processor.MailProcessor.read_mails') @patch('kairon.events.definitions.mail_channel.MailProcessEvent.enqueue') @patch('kairon.events.definitions.mail_channel.MailProcessEvent.validate') def test_mail_read_event_execute(self, mock_validate, mock_enqueue, mock_read_mails): from kairon.events.definitions.mail_channel import MailReadEvent bot = "test_add_schedule_event" user = "test_user" mock_read_mails.return_value = (["[email protected]"], user, 10) mock_validate.return_value = True event = MailReadEvent(bot, user) event.execute() mock_read_mails.assert_called_once_with(bot) mock_validate.assert_called_once() mock_enqueue.assert_called_once_with(mails=["[email protected]"]) + # Add assertions to verify event properties + assert event.bot == bot + assert event.user == user
1324-1333: Improve error handling test coverageAdd more test cases for different error scenarios.
def test_mail_read_event_execute_validation_failed(self): bot = "test_add_schedule_event" user = "test_user" with patch('kairon.events.definitions.mail_channel.MailProcessEvent.validate', return_value=False): from kairon.events.definitions.mail_channel import MailReadEvent event = MailReadEvent(bot, user) with pytest.raises(AppException, match="Mail processing validation failed"): event.execute() def test_mail_read_event_execute_no_mails(self): bot = "test_add_schedule_event" user = "test_user" with patch('kairon.shared.channels.mail.processor.MailProcessor.read_mails', return_value=([], user, 0)): from kairon.events.definitions.mail_channel import MailReadEvent event = MailReadEvent(bot, user) event.execute() # Should handle empty mail list gracefullykairon/api/app/routers/bot/bot.py (2)
37-37: Remove unused importThe
MailConfigRequestimport is not used in this file.-from kairon.shared.data.data_models import MailConfigRequest🧰 Tools
🪛 Ruff (0.8.0)
37-37:
kairon.shared.data.data_models.MailConfigRequestimported but unusedRemove unused import:
kairon.shared.data.data_models.MailConfigRequest(F401)
1663-1664: Remove extra blank linesReduce multiple blank lines at the end of the file to a single blank line.
@router.get("/mail_channel/logs", response_model=Response) async def get_mail_channel_logs(start_idx: int = 0, page_size: int = 10, current_user: User = Security(Authentication.get_current_user_and_bot, scopes=TESTER_ACCESS)): data = MailProcessor.get_log(current_user.get_bot(), start_idx, page_size) return Response(data=data) - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
kairon/__init__.py(2 hunks)kairon/api/app/routers/bot/bot.py(3 hunks)kairon/chat/agent_processor.py(1 hunks)kairon/chat/utils.py(1 hunks)kairon/cli/mail_channel_process.py(1 hunks)kairon/cli/mail_channel_read.py(1 hunks)kairon/events/definitions/factory.py(2 hunks)kairon/events/definitions/mail_channel.py(1 hunks)kairon/events/server.py(2 hunks)kairon/events/utility.py(2 hunks)kairon/shared/channels/mail/constants.py(1 hunks)kairon/shared/channels/mail/data_objects.py(1 hunks)kairon/shared/channels/mail/processor.py(1 hunks)kairon/shared/channels/mail/scheduler.py(1 hunks)kairon/shared/chat/processor.py(1 hunks)kairon/shared/constants.py(3 hunks)kairon/shared/data/data_models.py(1 hunks)metadata/integrations.yml(1 hunks)requirements/prod.txt(1 hunks)system.yaml(3 hunks)tests/integration_test/action_service_test.py(2 hunks)tests/integration_test/event_service_test.py(1 hunks)tests/integration_test/services_test.py(6 hunks)tests/testing_data/system.yaml(2 hunks)tests/unit_test/channels/mail_channel_test.py(1 hunks)tests/unit_test/channels/mail_scheduler_test.py(1 hunks)tests/unit_test/chat/chat_test.py(2 hunks)tests/unit_test/cli_test.py(2 hunks)tests/unit_test/data_processor/agent_processor_test.py(1 hunks)tests/unit_test/data_processor/data_processor_test.py(19 hunks)tests/unit_test/events/definitions_test.py(1 hunks)tests/unit_test/utility_test.py(1 hunks)
🔥 Files not summarized due to errors (2)
- tests/integration_test/action_service_test.py: Error: Server error: no LLM provider could handle the message
- tests/unit_test/data_processor/data_processor_test.py: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 Ruff (0.8.0)
tests/integration_test/action_service_test.py
6-6: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
tests/unit_test/data_processor/data_processor_test.py
3992-3992: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
3994-3994: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7137-7137: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7139-7139: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7282-7282: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7284-7284: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7347-7347: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7349-7349: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7397-7397: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7399-7399: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7455-7455: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7457-7457: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7500-7500: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
7502-7502: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
kairon/api/app/routers/bot/bot.py
37-37: kairon.shared.data.data_models.MailConfigRequest imported but unused
Remove unused import: kairon.shared.data.data_models.MailConfigRequest
(F401)
1666-1666: Redefinition of unused get_action_server_logs from line 826
(F811)
1667-1668: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
kairon/events/definitions/mail_channel.py
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
98-98: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
99-99: Local variable is_initialized is assigned to but never used
Remove assignment to unused variable is_initialized
(F841)
tests/unit_test/channels/mail_scheduler_test.py
8-8: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
tests/unit_test/cli_test.py
323-323: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
345-349: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
kairon/shared/channels/mail/data_objects.py
4-4: mongoengine.BooleanField imported but unused
Remove unused import: mongoengine.BooleanField
(F401)
5-5: kairon.exceptions.AppException imported but unused
Remove unused import: kairon.exceptions.AppException
(F401)
7-7: kairon.shared.data.signals.auditlog imported but unused
Remove unused import
(F401)
7-7: kairon.shared.data.signals.push_notification imported but unused
Remove unused import
(F401)
kairon/events/server.py
151-151: 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
1-1: asyncio imported but unused
Remove unused import: asyncio
(F401)
9-9: uuid6.uuid7 imported but unused
Remove unused import: uuid6.uuid7
(F401)
14-14: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
431-432: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
431-431: Local variable mock_save is assigned to but never used
Remove assignment to unused variable mock_save
(F841)
kairon/events/utility.py
61-61: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
71-71: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
kairon/cli/mail_channel_read.py
1-1: json imported but unused
Remove unused import: json
(F401)
6-6: kairon.events.definitions.mail_channel.MailProcessEvent imported but unused
Remove unused import: kairon.events.definitions.mail_channel.MailProcessEvent
(F401)
kairon/shared/channels/mail/processor.py
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
163-163: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
218-218: 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/events/definitions_test.py
1214-1215: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1214-1214: Local variable mp is assigned to but never used
Remove assignment to unused variable mp
(F841)
1215-1215: Local variable mock_login is assigned to but never used
Remove assignment to unused variable mock_login
(F841)
1216-1216: Local variable mock_logout is assigned to but never used
Remove assignment to unused variable mock_logout
(F841)
1258-1258: unittest.mock.patch imported but unused
Remove unused import: unittest.mock.patch
(F401)
1286-1288: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (28)
tests/integration_test/action_service_test.py (1)
24-25: LGTM!
The empty lines appropriately separate different groups of imports, improving readability.
tests/unit_test/data_processor/data_processor_test.py (3)
3883-3889: LGTM: Mail-related domain configuration looks good
The test correctly verifies the addition of mail-related slots (mail_id, subject, body) to both domain slots and entities list, which aligns with the mail channel implementation objective.
4456-4456: LGTM: Mail entities verification is complete
The test correctly verifies all required mail-related entities (mail_id, subject, body) are present in the domain configuration.
9378-9387: LGTM: Mail slot configurations look good
The test correctly verifies the mail-related slots (mail_id, subject, body) with appropriate configurations:
- All slots are marked as default
- All slots influence conversation
- Correct type (text) is set for all mail slots
Let's verify that these slot configurations match the actual implementation:
✅ Verification successful
Mail slot configurations are correctly implemented
Based on the verification results, the mail-related slots (mail_id, subject, body) are consistently defined across the codebase:
- They are properly registered as entities in the domain
- They are included in the expected slot configurations with correct properties
- The test assertions verify both their presence and configurations
- The slots are used in mail channel tests with appropriate test data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mail slot definitions in the codebase
ast-grep --pattern 'Slots.objects.create(
name=$_,
type="text",
influence_conversation=True,
is_default=True
)'
Length of output: 121
Script:
#!/bin/bash
# Let's try a different approach to search for mail-related slot definitions
rg -A 5 "mail_id|subject|body" --type python | grep -i "slot"
Length of output: 92
Script:
#!/bin/bash
# Search for slot configurations in test files
rg -A 5 "'name':\s*'(mail_id|subject|body)'" --type python
Length of output: 103
Script:
#!/bin/bash
# Look for slot definitions in Python files
ast-grep --pattern 'slot = {
$$$
"name": $_,
$$$
}'
Length of output: 1047
Script:
#!/bin/bash
# Let's try searching without file type restriction
rg -A 5 "'name':\s*'(mail_id|subject|body)'"
Length of output: 1116
Script:
#!/bin/bash
# Search for mail-related slots in a different way
rg "mail_id.*subject.*body" -A 10 -B 10
Length of output: 10039
Script:
#!/bin/bash
# Look for slot definitions in the test file directly
cat "tests/unit_test/data_processor/data_processor_test.py" | grep -A 10 -B 10 "mail_id"
Length of output: 7778
kairon/shared/channels/mail/processor.py (2)
119-122: Avoid swallowing exceptions in send_mail
In the send_mail method, exceptions are caught, and the status is updated to FAILED, but the exception is not re-raised or logged at an appropriate level. Consider logging the exception with stack trace or re-raising it to ensure that critical failures are not silently ignored.
Ensure that exceptions in send_mail are properly handled:
except Exception as e:
logger.error(f"Error sending mail to {to}: {str(e)}", exc_info=True)
mail_log = MailResponseLog.objects.get(id=log_id)
mail_log.status = MailStatus.FAILED.value
mail_log.save()
# Consider re-raising if necessary
# raise AppException(f"Error sending mail to {to}") from e132-133: Handle empty responses in process_mail
If there are no responses from rasa_chat_response, the method returns an empty string. Ensure that the calling code can handle this scenario appropriately, or consider raising an exception if this is unexpected.
tests/unit_test/utility_test.py (1)
2851-2853: Update expected channels in tests
The addition of "line" and "mail" channels in the test_get_channels method should be reflected throughout the test suite to ensure consistency. Verify that all tests and documentation are updated accordingly.
tests/integration_test/services_test.py (6)
4409-4410: LGTM: Improved readability with blank lines
4751-4751: LGTM: Improved readability with blank line
9068-9069: LGTM: Mail-related entities properly added
The new entities (mail_id, subject, body) are consistent with the mail channel feature implementation.
23869-23871: LGTM: Improved readability with blank lines
8301-8301: Verify the updated entity count
The assertion has been updated to expect 17 entities, which aligns with the addition of three new mail-related entities (mail_id, subject, body).
9707-9712: Verify the updated slot counts
The assertions have been updated to reflect:
- Total slots increased to 24 (from 21)
- Default slots increased to 17 (from 14)
These changes align with the addition of three new mail-related slots.
✅ Verification successful
Let me gather more information to verify the slot counts and mail-related slots.
Let me search for the slot definitions more specifically.
Let me search for the slot definitions in a more focused way.
Let me search for the total slot count and mail-related slots in a more targeted way.
Based on the search results, I can now generate the final response.
Slot count assertions are verified
The assertions for total slots (24) and default slots (17) are correct and align with the addition of three mail-related slots. This is evidenced by:
- The total slot count increased from 21 to 24 slots
- The default slot count increased from 14 to 17 slots
- The changes are consistent with the addition of mail-related slots:
- smtp_url
- smtp_port
- smtp_password
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the slot definitions and counts
# Search for slot definitions
rg -A 10 "default_slots|DEFAULT_SLOTS"
# Search for mail-related slot configurations
ast-grep --pattern 'slot.$$$mail'
Length of output: 920
Script:
#!/bin/bash
# Search for total slots count assertions
rg "len\(actual\[.data.\]\)|assert len\(.*\) == \d+"
# Search for mail-related slots in a broader context
rg -i "mail.*slot|slot.*mail"
# Look for slot definitions or configurations
rg "slot.*type|type.*slot" -A 3
Length of output: 66100
Script:
#!/bin/bash
# Search for slot definitions and configurations
rg -B 5 -A 5 "slots.*=.*\[|slots:.*\[|DEFAULT_SLOTS.*=.*\["
# Search for mail-related configurations in a broader context
rg -B 3 -A 3 "mail.*config|email.*config"
Length of output: 75816
Script:
#!/bin/bash
# Search for slot definitions in domain files and configuration
rg -B 2 -A 2 "slots:|DEFAULT_SLOTS =|slots ="
# Search specifically for mail-related slots
rg -B 2 -A 2 "mail.*slot|email.*slot|smtp.*slot"
Length of output: 70976
Script:
#!/bin/bash
# Search for total slot count assertion
rg -B 2 -A 2 "assert len\(actual\[.data.\]\) == \d+|assert len\(slots\) == \d+"
# Search for mail/email/smtp related configurations in domain files
rg -B 2 -A 2 "mail|email|smtp" tests/integration_test/services_test.py domain.yml
Length of output: 84840
kairon/cli/mail_channel_read.py (2)
13-29: LGTM! Well-structured CLI implementation.
The command-line interface is well-implemented with:
- Clear help messages
- Proper argument validation
- Standard argument parser configuration
9-10: Verify error handling in execute().
The execute() method is called without visible error handling.
✅ Verification successful
Error handling is properly implemented in execute()
The execute() method in MailReadEvent class has proper error handling:
- It uses a try-catch block to handle exceptions
- Catches all exceptions and raises them as
AppExceptionwith a descriptive error message - Includes the bot identifier and original error in the exception message for better debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in MailReadEvent.execute()
ast-grep --pattern 'class MailReadEvent {
$$$
execute() {
$$$
}
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Search for MailReadEvent class implementation using ripgrep
rg -l "class MailReadEvent" --type py
# Once found, show the content of files containing MailReadEvent
rg "class MailReadEvent" -A 30 --type py
Length of output: 2275
Script:
#!/bin/bash
# Get more context for execute method implementation
rg "def execute" -A 20 -B 2 kairon/events/definitions/mail_channel.py
Length of output: 1238
kairon/shared/channels/mail/scheduler.py (2)
28-29: LGTM! Clear error handling.
The error handling is well-implemented with descriptive error messages.
19-27: Verify URL construction security.
The URL construction looks secure, but let's verify the implementation of get_event_server_url() and URL joining.
✅ Verification successful
Based on the code analysis, I can now provide a final response regarding the URL construction security:
URL construction is secure and properly validated
The implementation shows several security measures:
get_event_server_url()validates the URL from a secure configuration and raises an exception if not foundurljoin()is used for safe URL path joining instead of string concatenationexecute_http_request()includes proper validation and error handling- The bot parameter is passed as part of the URL path without any manipulation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL validation in Utility class
ast-grep --pattern 'class Utility {
$$$
get_event_server_url() {
$$$
}
$$$
}'
# Search for URL validation in execute_http_request
ast-grep --pattern 'class Utility {
$$$
execute_http_request($$$) {
$$$
}
$$$
}'
Length of output: 186
Script:
#!/bin/bash
# Search for Utility class implementation with ripgrep
rg -l "class Utility" --type py
# After finding the file, let's look at the implementation
rg "get_event_server_url|execute_http_request" -A 10 --type py
Length of output: 66905
kairon/events/definitions/factory.py (1)
24-26: LGTM! Clean event registration
The new mail channel events are properly registered following the existing pattern.
kairon/cli/mail_channel_process.py (1)
16-36: Consider adding validation for bot and user IDs
The command accepts bot and user IDs without validation. Consider adding basic format validation.
kairon/__init__.py (1)
66-67: LGTM: New subparser additions
The addition of mail channel subparsers follows the existing pattern and is implemented correctly.
kairon/events/utility.py (1)
3-11: LGTM: Import statements are properly organized
The new imports are correctly placed and necessary for the added functionality.
tests/unit_test/channels/mail_scheduler_test.py (2)
14-30: LGTM: Well-structured test fixture
The fixture properly mocks all required dependencies and provides a clean test environment.
33-54: LGTM: Comprehensive test coverage for request_epoch
The success and failure scenarios are well tested with appropriate mocking.
tests/testing_data/system.yaml (2)
124-124: LGTM: Mail queue configuration
The mail queue configuration is properly structured under the events queue section.
144-144: LGTM: Mail scheduler configuration
The mail scheduler configuration is properly placed under the events scheduler section.
system.yaml (2)
130-130: LGTM: Mail configurations match test environment
The mail queue and scheduler configurations are consistent with the test environment.
Also applies to: 145-145
158-158: Verify the evaluator endpoint change
The pyscript evaluator endpoint has been changed to localhost. Ensure this is intentional and won't affect production deployments.
✅ Verification successful
Let me gather more context about the configuration patterns and deployment setup.
The localhost endpoint configuration appears to be the intended default
The change to use localhost as the default endpoint is consistent across the codebase. The configuration is designed to be overridden via the PYSCRIPT_EVALUATOR_ENDPOINT environment variable in production deployments:
system.yaml: Default endpoint set tolocalhost:8087tests/testing_data/system.yaml: Test configuration withlocalhost:8080
No references to hardcoded production endpoints were found, indicating the system properly relies on environment variable configuration for different deployment environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the evaluator endpoint is referenced in other configurations or documentation
# Search for any references to the old IP
rg -l "192.168.100.109:8087"
# Search for other evaluator endpoint configurations
rg "PYSCRIPT_EVALUATOR_ENDPOINT"
Length of output: 250
Script:
#!/bin/bash
# Check for deployment-related files and environment configurations
fd -e yaml -e yml -e env -e template --exec grep -l "PYSCRIPT_EVALUATOR" {} \;
# Look for documentation about deployment configuration
fd README -e md | xargs grep -l "PYSCRIPT_EVALUATOR"
# Check for any deployment scripts or kubernetes manifests
fd -e yaml -e yml --exec grep -l "pyscript.*evaluator" {} \;
Length of output: 294
kairon/shared/chat/processor.py (1)
42-44: Verify test coverage for mail channel configuration.
The new mail channel handling code path should be properly tested.
✅ Verification successful
Mail channel configuration is well tested with comprehensive test coverage.
The codebase shows extensive test coverage for mail channel functionality:
-
Unit tests in
tests/unit_test/channels/mail_channel_test.pycover:- Mail sending functionality
- Mail processing and reading
- IMAP connection handling
- SMTP connection management
- Channel configuration validation
-
Integration tests in
tests/integration_test/action_service_test.pycover:- Email action execution with various scenarios
- Template handling
- Error cases
- Configuration validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relevant test files and coverage
fd "test.*processor\.py" | xargs rg -A 5 "def test.*mail.*config"
# Check for existing mail channel test scenarios
rg -A 10 "ChannelTypes.MAIL" "tests/"
Length of output: 14563
| @@ -2,6 +2,10 @@ | |||
| import datetime | |||
| import os | |||
| from urllib.parse import urlencode, urljoin | |||
| from kairon.shared.utils import Utility | |||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use uppercase for environment variable names
Following Python conventions, environment variable names should be uppercase.
-os.environ["system_file"] = "./tests/testing_data/system.yaml"
+os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"📝 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.
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" |
🧰 Tools
🪛 Ruff (0.8.0)
6-6: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
| except Exception as e: | ||
| logger.error(str(e)) | ||
| raise AppException(e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining when re-raising exceptions
When re-raising exceptions within an except block, use raise ... from e to preserve the original traceback and exception context. This aids in debugging by maintaining the exception chain.
Apply this diff to correctly raise exceptions:
# In all except blocks where exceptions are re-raised, modify as follows:
- raise AppException(e)
+ raise AppException(e) from eAlso applies to: 49-51, 79-82, 96-98
🧰 Tools
🪛 Ruff (0.8.0)
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| except Exception as e: | ||
| raise AppException(str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining when re-raising exceptions
When re-raising exceptions within an except block, use raise ... from e to maintain the original traceback and exception context, aiding in debugging.
Apply this diff to correctly raise exceptions:
# In all except blocks where exceptions are re-raised, modify as follows:
- raise AppException(str(e))
+ raise AppException(str(e)) from eAlso applies to: 162-163, 217-218
🧰 Tools
🪛 Ruff (0.8.0)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
kairon/cli/mail_channel_read.py
Outdated
| @@ -0,0 +1,29 @@ | |||
| import json | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports.
The following imports are not used in the code:
jsonMailProcessEvent
Also applies to: 6-6
🧰 Tools
🪛 Ruff (0.8.0)
1-1: json imported but unused
Remove unused import: json
(F401)
| @staticmethod | ||
| async def process_messages_via_bot( | ||
| messages: [str], | ||
| account: int, | ||
| bot: str, | ||
| user: str, | ||
| is_integration_user: bool = False, | ||
| metadata: Dict = None, | ||
| ): | ||
| """Process a list of messages through the bot. | ||
| Args: | ||
| messages: List of messages to process | ||
| account: Account ID | ||
| bot: Bot ID | ||
| user: User ID | ||
| is_integration_user: Flag indicating if user is integration user | ||
| metadata: Additional metadata | ||
|
|
||
| Returns: | ||
| List of responses from the bot | ||
| """ | ||
| responses = [] | ||
| uncached_model = AgentProcessor.get_agent_without_cache(bot, False) | ||
| metadata = ChatUtils.get_metadata(account, bot, is_integration_user, metadata) | ||
| for message in messages: | ||
| msg = UserMessage(message, sender_id=user, metadata=metadata) | ||
| chat_response = await uncached_model.handle_message(msg) | ||
| responses.append(chat_response) | ||
| return responses | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The method needs the following improvements:
- Add error handling for message processing failures
- Validate that the message list is non-empty
- Add return type hint for better code maintainability
Apply this diff to implement the suggested improvements:
@staticmethod
async def process_messages_via_bot(
messages: [str],
account: int,
bot: str,
user: str,
is_integration_user: bool = False,
metadata: Dict = None,
- ):
+ ) -> List[Dict]:
+ if not messages:
+ raise ValueError("Message list cannot be empty")
responses = []
uncached_model = AgentProcessor.get_agent_without_cache(bot, False)
metadata = ChatUtils.get_metadata(account, bot, is_integration_user, metadata)
for message in messages:
+ try:
msg = UserMessage(message, sender_id=user, metadata=metadata)
chat_response = await uncached_model.handle_message(msg)
responses.append(chat_response)
+ except Exception as e:
+ logger.exception(f"Error processing message: {message}")
+ responses.append({"error": str(e), "message": message})
return responsesDon't forget to add the following import:
from typing import List| with patch('kairon.shared.channels.mail.processor.MailProcessor.__init__', return_value=None) as mp: | ||
| with patch('kairon.shared.channels.mail.processor.MailProcessor.login_smtp', return_value=None) as mock_login: | ||
| with patch('kairon.shared.channels.mail.processor.MailProcessor.logout_smtp', return_value=None) as mock_logout: | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested context managers and remove unused variables
Combine the nested context managers and remove unused mock variables.
- with patch('kairon.shared.channels.mail.processor.MailProcessor.__init__', return_value=None) as mp:
- with patch('kairon.shared.channels.mail.processor.MailProcessor.login_smtp', return_value=None) as mock_login:
- with patch('kairon.shared.channels.mail.processor.MailProcessor.logout_smtp', return_value=None) as mock_logout:
+ with (
+ patch('kairon.shared.channels.mail.processor.MailProcessor.__init__', return_value=None),
+ patch('kairon.shared.channels.mail.processor.MailProcessor.login_smtp', return_value=None),
+ patch('kairon.shared.channels.mail.processor.MailProcessor.logout_smtp', return_value=None)
+ ):📝 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.
| with patch('kairon.shared.channels.mail.processor.MailProcessor.__init__', return_value=None) as mp: | |
| with patch('kairon.shared.channels.mail.processor.MailProcessor.login_smtp', return_value=None) as mock_login: | |
| with patch('kairon.shared.channels.mail.processor.MailProcessor.logout_smtp', return_value=None) as mock_logout: | |
| with ( | |
| patch('kairon.shared.channels.mail.processor.MailProcessor.__init__', return_value=None), | |
| patch('kairon.shared.channels.mail.processor.MailProcessor.login_smtp', return_value=None), | |
| patch('kairon.shared.channels.mail.processor.MailProcessor.logout_smtp', return_value=None) | |
| ): |
🧰 Tools
🪛 Ruff (0.8.0)
1214-1215: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1214-1214: Local variable mp is assigned to but never used
Remove assignment to unused variable mp
(F841)
1215-1215: Local variable mock_login is assigned to but never used
Remove assignment to unused variable mock_login
(F841)
1216-1216: Local variable mock_logout is assigned to but never used
Remove assignment to unused variable mock_logout
(F841)
| with patch("kairon.shared.channels.mail.processor.MailProcessor.process_message_task", | ||
| side_effect=Exception("Test")): | ||
| with pytest.raises(AppException, match="Test"): | ||
| MailProcessEvent("", "").execute(mails=["[email protected]"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested context managers
Combine the nested context managers for better readability.
- with patch("kairon.shared.channels.mail.processor.MailProcessor.process_message_task",
- side_effect=Exception("Test")):
- with pytest.raises(AppException, match="Test"):
- MailProcessEvent("", "").execute(mails=["[email protected]"])
+ with (
+ patch("kairon.shared.channels.mail.processor.MailProcessor.process_message_task",
+ side_effect=Exception("Test")),
+ pytest.raises(AppException, match="Test")
+ ):
+ MailProcessEvent("", "").execute(mails=["[email protected]"])📝 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.
| with patch("kairon.shared.channels.mail.processor.MailProcessor.process_message_task", | |
| side_effect=Exception("Test")): | |
| with pytest.raises(AppException, match="Test"): | |
| MailProcessEvent("", "").execute(mails=["[email protected]"]) | |
| with ( | |
| patch("kairon.shared.channels.mail.processor.MailProcessor.process_message_task", | |
| side_effect=Exception("Test")), | |
| pytest.raises(AppException, match="Test") | |
| ): | |
| MailProcessEvent("", "").execute(mails=["[email protected]"]) |
🧰 Tools
🪛 Ruff (0.8.0)
1286-1288: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
| if configuration['connector_type'] == ChannelTypes.MAIL.value: | ||
| from kairon.shared.channels.mail.scheduler import MailScheduler | ||
| MailScheduler.request_epoch(bot) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for mail scheduler initialization.
While the lazy import strategy is good for avoiding circular dependencies, the request_epoch call should include error handling to gracefully handle potential initialization failures.
Consider wrapping the scheduler initialization in a try-except block:
if configuration['connector_type'] == ChannelTypes.MAIL.value:
from kairon.shared.channels.mail.scheduler import MailScheduler
- MailScheduler.request_epoch(bot)
+ try:
+ MailScheduler.request_epoch(bot)
+ except Exception as e:
+ logger.error(f"Failed to initialize mail scheduler for bot {bot}: {str(e)}")
+ raise AppException(f"Failed to initialize mail channel: {str(e)}")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
kairon/events/utility.py (1)
67-70: Add pagination for bot retrievalWhen retrieving bots, implement pagination to handle large datasets efficiently.
Apply this diff:
try: - bots = list(Channels.objects(connector_type=ChannelTypes.MAIL.value).distinct("bot")) - for bot in bots: - logger.info(f"Rescheduling mail reading for bot {bot}") - EventUtility.schedule_channel_mail_reading(bot) + page_size = 100 + offset = 0 + while True: + bots = list(Channels.objects(connector_type=ChannelTypes.MAIL.value) + .distinct("bot").skip(offset).limit(page_size)) + if not bots: + break + for bot in bots: + logger.info(f"Rescheduling mail reading for bot {bot}") + EventUtility.schedule_channel_mail_reading(bot) + offset += page_sizekairon/shared/channels/mail/processor.py (4)
22-33: Add type hints to improve code maintainabilityConsider adding type hints to the constructor parameters and instance variables for better code maintainability and IDE support.
- def __init__(self, bot): + def __init__(self, bot: str) -> None: self.bot: str = bot self.config: dict = ChatDataProcessor.get_channel_config(ChannelTypes.MAIL, bot, False)['config'] self.intent: str = self.config.get('intent') self.mail_template: str = self.config.get('mail_template', MailConstants.DEFAULT_TEMPLATE) self.bot_settings: BotSettings = BotSettings.objects(bot=self.bot).get() self.state: MailChannelStateData = MailProcessor.get_mail_channel_state_data(bot) bot_info: Bot = Bot.objects.get(id=bot) self.account: str = bot_info.account self.mailbox: Optional[MailBox] = None self.smtp: Optional[smtplib.SMTP] = None
52-81: Use context managers for connection handlingConsider implementing context managers (
__enter__and__exit__) to ensure proper resource cleanup, especially for IMAP and SMTP connections.def __enter__(self): self.login_imap() self.login_smtp() return self def __exit__(self, exc_type, exc_val, exc_tb): self.logout_imap() self.logout_smtp()Usage example:
with MailProcessor(bot) as mp: # Your code here pass # Connections are automatically closed
83-104: Add docstrings to validation methodsAdd docstrings to describe the purpose, parameters, and return values of the validation methods.
@staticmethod def validate_smtp_connection(bot): + """ + Validates SMTP connection for the given bot. + + Args: + bot (str): The bot identifier + + Returns: + bool: True if connection is successful, False otherwise + """
231-297: Improve return value documentationThe method returns a tuple but the type hints and documentation could be clearer about the structure.
@staticmethod - def read_mails(bot: str) -> ([dict], str, int): + def read_mails(bot: str) -> tuple[list[dict], str, int]: """ Read mails from the mailbox Parameters: - bot: str - bot id Returns: - - list of messages - each message is a dict with the following + tuple containing: + 1. list[dict]: List of messages, where each message contains: - mail_id - subject - date - body - log_id - - user - - time_shift + 2. str: user identifier + 3. int: time shift in seconds """tests/unit_test/channels/mail_channel_test.py (3)
1-1: Remove unused importsThe following imports are not used in the code:
asynciouuid7fromuuid6-import asyncio import os from unittest.mock import patch, MagicMock import pytest from imap_tools import MailMessage from mongoengine import connect, disconnect -from uuid6 import uuid7Also applies to: 9-9
🧰 Tools
🪛 Ruff (0.8.0)
1-1:
asyncioimported but unusedRemove unused import:
asyncio(F401)
14-14: Use uppercase for environment variablesFollow Python conventions by using uppercase for environment variables.
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.0)
14-14: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
484-486: Simplify nested context managersUse a single
withstatement with multiple contexts instead of nesting them.- with patch.object(MailChannelStateData, 'save', return_value=None) as mock_save: - with patch('kairon.shared.channels.mail.data_objects.MailChannelStateData', return_value=mock_state): + with patch.object(MailChannelStateData, 'save', return_value=None) as mock_save, \ + patch('kairon.shared.channels.mail.data_objects.MailChannelStateData', return_value=mock_state):🧰 Tools
🪛 Ruff (0.8.0)
484-485: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
484-484: Local variable
mock_saveis assigned to but never usedRemove assignment to unused variable
mock_save(F841)
kairon/api/app/routers/bot/bot.py (1)
37-37: Remove unused importThe
MailConfigRequestimport is not used in the code.-from kairon.shared.data.data_models import MailConfigRequest🧰 Tools
🪛 Ruff (0.8.0)
37-37:
kairon.shared.data.data_models.MailConfigRequestimported but unusedRemove unused import:
kairon.shared.data.data_models.MailConfigRequest(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
kairon/api/app/routers/bot/bot.py(3 hunks)kairon/cli/mail_channel_read.py(1 hunks)kairon/events/definitions/mail_channel.py(1 hunks)kairon/events/utility.py(2 hunks)kairon/shared/channels/mail/data_objects.py(1 hunks)kairon/shared/channels/mail/processor.py(1 hunks)tests/unit_test/channels/mail_channel_test.py(1 hunks)tests/unit_test/channels/mail_scheduler_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kairon/cli/mail_channel_read.py
- kairon/shared/channels/mail/data_objects.py
🧰 Additional context used
🪛 Ruff (0.8.0)
kairon/events/definitions/mail_channel.py
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
98-98: 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
8-8: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
kairon/api/app/routers/bot/bot.py
37-37: kairon.shared.data.data_models.MailConfigRequest imported but unused
Remove unused import: kairon.shared.data.data_models.MailConfigRequest
(F401)
1667-1668: Do not perform function call Security 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
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
219-219: 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/utility.py
62-62: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
72-72: 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
1-1: asyncio imported but unused
Remove unused import: asyncio
(F401)
9-9: uuid6.uuid7 imported but unused
Remove unused import: uuid6.uuid7
(F401)
14-14: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
484-485: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
484-484: Local variable mock_save is assigned to but never used
Remove assignment to unused variable mock_save
(F841)
🔇 Additional comments (12)
kairon/events/definitions/mail_channel.py (3)
97-98:
Use exception chaining for better error tracking
When re-raising exceptions, use exception chaining to preserve the original traceback.
Apply this diff:
except Exception as e:
- raise AppException(f"Failed to schedule mail reading for bot {self.bot}. Error: {str(e)}")
+ raise AppException(f"Failed to schedule mail reading for bot {self.bot}. Error: {str(e)}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
98-98: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-51:
Use exception chaining for better error tracking
When re-raising exceptions, use exception chaining to preserve the original traceback.
Apply this diff:
except Exception as e:
logger.error(str(e))
- raise AppException(e)
+ raise AppException(e) from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
37-39:
Use exception chaining for better error tracking
When re-raising exceptions, use exception chaining to preserve the original traceback.
Apply this diff:
except Exception as e:
logger.error(str(e))
- raise AppException(e)
+ raise AppException(e) from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
39-39: 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/utility.py (2)
61-62:
Use exception chaining for better error tracking
When re-raising exceptions, use exception chaining to preserve the original traceback.
Apply this diff:
except Exception as e:
- raise AppException(f"Failed to schedule mail reading for bot {bot}. Error: {str(e)}")
+ raise AppException(f"Failed to schedule mail reading for bot {bot}. Error: {str(e)}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
62-62: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
46-47: 🛠️ Refactor suggestion
Add validation for interval value
The interval value should be validated to ensure it's a positive integer.
Apply this diff:
mail_processor = MailProcessor(bot)
interval = mail_processor.config.get("interval", 60)
+ if not isinstance(interval, int) or interval < 1:
+ raise ValueError(f"Invalid interval value: {interval}. Must be a positive integer.")Likely invalid or redundant comment.
tests/unit_test/channels/mail_scheduler_test.py (3)
33-66: LGTM! Comprehensive test coverage for request_epoch
The test cases thoroughly cover success and failure scenarios with proper mocking.
69-96: LGTM! Well-structured tests for schedule_channel_mail_reading
The test cases effectively verify both scenarios (with and without existing event_id) and include proper assertions.
112-125: LGTM! Good test coverage for reschedule_all_bots_channel_mail_reading
The test effectively verifies the scheduling of multiple bots with proper assertions.
kairon/shared/channels/mail/processor.py (3)
49-50: Use exception chaining to preserve error context
When re-raising exceptions, use exception chaining to maintain the original traceback.
- raise AppException(str(e))
+ raise AppException(str(e)) from e🧰 Tools
🪛 Ruff (0.8.0)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
163-164: Use exception chaining to preserve error context
When re-raising exceptions, use exception chaining to maintain the original traceback.
- raise AppException(str(e))
+ raise AppException(str(e)) from e🧰 Tools
🪛 Ruff (0.8.0)
164-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
218-219: Use exception chaining to preserve error context
When re-raising exceptions, use exception chaining to maintain the original traceback.
- raise AppException(str(e))
+ raise AppException(str(e)) from e🧰 Tools
🪛 Ruff (0.8.0)
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
kairon/api/app/routers/bot/bot.py (1)
1665-1673: 🛠️ Refactor suggestion
Move Security call inside the function
Don't use Security in default arguments. Move it inside the function body.
@router.get("/mail_channel/logs", response_model=Response)
-async def get_mail_channel_logs(start_idx: int = 0, page_size: int = 10,
- current_user: User = Security(Authentication.get_current_user_and_bot,
- scopes=TESTER_ACCESS)):
+async def get_mail_channel_logs(
+ start_idx: int = 0,
+ page_size: int = 10
+):
+ current_user = Security(Authentication.get_current_user_and_bot, scopes=TESTER_ACCESS)
"""
Retrieves mail channel related logs for the bot.
"""
data = MailProcessor.get_log(current_user.get_bot(), start_idx, page_size)
return Response(data=data)Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
1667-1668: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| """ | ||
| try: | ||
| vals = MailProcessor.read_mails(self.bot) | ||
| print(vals) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace print statement with logger
Use logger instead of print for consistent logging across the application.
Apply this diff:
- print(vals)
+ logger.debug(f"Mail read values: {vals}")Committable suggestion skipped: line range outside the PR's diff.
| from kairon import Utility | ||
| from kairon.exceptions import AppException | ||
|
|
||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use uppercase for environment variables
Environment variables should be in uppercase by convention.
Apply this diff:
-os.environ["system_file"] = "./tests/testing_data/system.yaml"
+os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"📝 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.
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" |
🧰 Tools
🪛 Ruff (0.8.0)
8-8: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
| async def send_mail(self, to: str, subject: str, body: str, log_id: str): | ||
| try: | ||
| if body and len(body) > 0: | ||
| email_account = self.config['email_account'] | ||
| msg = MIMEMultipart() | ||
| msg['From'] = email_account | ||
| msg['To'] = to | ||
| msg['Subject'] = subject | ||
| msg.attach(MIMEText(body, 'html')) | ||
| self.smtp.sendmail(email_account, to, msg.as_string()) | ||
| mail_log = MailResponseLog.objects.get(id=log_id) | ||
| mail_log.status = MailStatus.SUCCESS.value | ||
| mail_log.save() | ||
| except Exception as e: | ||
| logger.error(f"Error sending mail to {to}: {str(e)}") | ||
| mail_log = MailResponseLog.objects.get(id=log_id) | ||
| mail_log.status = MailStatus.FAILED.value | ||
| mail_log.responses.append(str(e)) | ||
| mail_log.save() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and input validation in send_mail
- Add input validation for email addresses
- Handle specific SMTP exceptions separately
- Add logging for successful sends
async def send_mail(self, to: str, subject: str, body: str, log_id: str):
try:
+ if not to or '@' not in to:
+ raise ValueError(f"Invalid email address: {to}")
if body and len(body) > 0:
email_account = self.config['email_account']
msg = MIMEMultipart()
msg['From'] = email_account
msg['To'] = to
msg['Subject'] = subject
msg.attach(MIMEText(body, 'html'))
self.smtp.sendmail(email_account, to, msg.as_string())
+ logger.info(f"Successfully sent email to {to}")
mail_log = MailResponseLog.objects.get(id=log_id)
mail_log.status = MailStatus.SUCCESS.value
mail_log.save()
except smtplib.SMTPException as e:
+ logger.error(f"SMTP error while sending mail to {to}: {str(e)}")
+ mail_log = MailResponseLog.objects.get(id=log_id)
+ mail_log.status = MailStatus.FAILED.value
+ mail_log.responses.append(f"SMTP error: {str(e)}")
+ mail_log.save()
except Exception as e:
logger.error(f"Error sending mail to {to}: {str(e)}")
mail_log = MailResponseLog.objects.get(id=log_id)
mail_log.status = MailStatus.FAILED.value
mail_log.responses.append(str(e))
mail_log.save()📝 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.
| async def send_mail(self, to: str, subject: str, body: str, log_id: str): | |
| try: | |
| if body and len(body) > 0: | |
| email_account = self.config['email_account'] | |
| msg = MIMEMultipart() | |
| msg['From'] = email_account | |
| msg['To'] = to | |
| msg['Subject'] = subject | |
| msg.attach(MIMEText(body, 'html')) | |
| self.smtp.sendmail(email_account, to, msg.as_string()) | |
| mail_log = MailResponseLog.objects.get(id=log_id) | |
| mail_log.status = MailStatus.SUCCESS.value | |
| mail_log.save() | |
| except Exception as e: | |
| logger.error(f"Error sending mail to {to}: {str(e)}") | |
| mail_log = MailResponseLog.objects.get(id=log_id) | |
| mail_log.status = MailStatus.FAILED.value | |
| mail_log.responses.append(str(e)) | |
| mail_log.save() | |
| async def send_mail(self, to: str, subject: str, body: str, log_id: str): | |
| try: | |
| if not to or '@' not in to: | |
| raise ValueError(f"Invalid email address: {to}") | |
| if body and len(body) > 0: | |
| email_account = self.config['email_account'] | |
| msg = MIMEMultipart() | |
| msg['From'] = email_account | |
| msg['To'] = to | |
| msg['Subject'] = subject | |
| msg.attach(MIMEText(body, 'html')) | |
| self.smtp.sendmail(email_account, to, msg.as_string()) | |
| logger.info(f"Successfully sent email to {to}") | |
| mail_log = MailResponseLog.objects.get(id=log_id) | |
| mail_log.status = MailStatus.SUCCESS.value | |
| mail_log.save() | |
| except smtplib.SMTPException as e: | |
| logger.error(f"SMTP error while sending mail to {to}: {str(e)}") | |
| mail_log = MailResponseLog.objects.get(id=log_id) | |
| mail_log.status = MailStatus.FAILED.value | |
| mail_log.responses.append(f"SMTP error: {str(e)}") | |
| mail_log.save() | |
| except Exception as e: | |
| logger.error(f"Error sending mail to {to}: {str(e)}") | |
| mail_log = MailResponseLog.objects.get(id=log_id) | |
| mail_log.status = MailStatus.FAILED.value | |
| mail_log.responses.append(str(e)) | |
| mail_log.save() |
| mail_log = MailResponseLog.objects.get(id=log_id) | ||
| mail_log.status = MailStatus.SUCCESS.value | ||
| mail_log.save() | ||
| except Exception as e: |
There was a problem hiding this comment.
catch specific exception
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
kairon/events/definitions/mail_channel.py (2)
22-26: Enhance validation in the validate methodThe current validation only checks SMTP connection. Consider validating:
- Required configuration parameters
- Mail template format
- Intent configuration
96-98: Enhance error handling with specific exceptionsConsider catching and handling specific exceptions (e.g., ConnectionError, TimeoutError) before catching the general Exception.
+ except ConnectionError as e: + raise AppException(f"Failed to connect to mail server for bot {self.bot}. Error: {str(e)}") from e + except TimeoutError as e: + raise AppException(f"Timeout while reading mails for bot {self.bot}. Error: {str(e)}") from e except Exception as e: raise AppException(f"Failed to schedule mail reading for bot {self.bot}. Error: {str(e)}")🧰 Tools
🪛 Ruff (0.8.0)
98-98: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
kairon/shared/channels/mail/processor.py (1)
230-296: Add retry mechanism for mail reading operationsThe read_mails method should implement a retry mechanism for transient failures.
@staticmethod - def read_mails(bot: str) -> ([dict], str, int): + async def read_mails(bot: str, max_retries: int = 3) -> ([dict], str, int): try: - mp.login_imap() + retry_count = 0 + while retry_count < max_retries: + try: + mp.login_imap() + break + except ConnectionError: + retry_count += 1 + if retry_count == max_retries: + raise + await asyncio.sleep(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
kairon/events/definitions/mail_channel.py(1 hunks)kairon/shared/channels/mail/processor.py(1 hunks)system.yaml(1 hunks)tests/integration_test/action_service_test.py(3 hunks)
🔥 Files not summarized due to errors (1)
- tests/integration_test/action_service_test.py: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- system.yaml
🧰 Additional context used
🪛 Ruff (0.8.0)
tests/integration_test/action_service_test.py
6-6: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
kairon/shared/channels/mail/processor.py
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
218-218: 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/definitions/mail_channel.py
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
98-98: 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 (6)
tests/integration_test/action_service_test.py (3)
6-6: Use uppercase for environment variable names
Following Python conventions, environment variable names should be uppercase.
-os.environ["system_file"] = "./tests/testing_data/system.yaml"
+os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.0)
6-6: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
24-25: LGTM! Good code organization
The empty lines provide clear separation between external and internal package imports, improving code readability.
11758-11758: LGTM! Well-structured test implementation
The test function is well-named, clearly indicating its purpose, and properly uses mocking for external dependencies.
kairon/events/definitions/mail_channel.py (2)
89-90: 🛠️ Refactor suggestion
Replace print statement with logger
Use structured logging instead of print statements for consistency and better observability.
- print(vals)
+ logger.debug(f"Mail read values: {vals}")Likely invalid or redundant comment.
37-39: 🛠️ Refactor suggestion
Enhance exception handling with proper context chaining
Use exception chaining to preserve the original traceback for better debugging.
except Exception as e:
logger.error(str(e))
- raise AppException(e)
+ raise AppException(e) from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
kairon/shared/channels/mail/processor.py (1)
105-124:
Enhance email sending security and validation
The send_mail method needs additional security measures and input validation:
- Validate email address format
- Handle specific SMTP exceptions
- Implement rate limiting
async def send_mail(self, to: str, subject: str, body: str, log_id: str):
+ if not isinstance(to, str) or '@' not in to:
+ raise ValueError(f"Invalid email address: {to}")
try:
if body and len(body) > 0:
email_account = self.config['email_account']
+ if not hasattr(self, '_last_send_time'):
+ self._last_send_time = 0
+ current_time = time.time()
+ if current_time - self._last_send_time < 1: # Rate limit: 1 second
+ await asyncio.sleep(1)
msg = MIMEMultipart()
msg['From'] = email_account
msg['To'] = to
msg['Subject'] = subject
msg.attach(MIMEText(body, 'html'))
self.smtp.sendmail(email_account, to, msg.as_string())
+ self._last_send_time = time.time()
mail_log = MailResponseLog.objects.get(id=log_id)
mail_log.status = MailStatus.SUCCESS.value
mail_log.save()
+ except smtplib.SMTPException as e:
+ logger.error(f"SMTP error while sending mail to {to}: {str(e)}")
+ mail_log = MailResponseLog.objects.get(id=log_id)
+ mail_log.status = MailStatus.FAILED.value
+ mail_log.responses.append(f"SMTP error: {str(e)}")
+ mail_log.save()
except Exception as e:
logger.error(f"Error sending mail to {to}: {str(e)}")
mail_log = MailResponseLog.objects.get(id=log_id)
mail_log.status = MailStatus.FAILED.value
mail_log.responses.append(str(e))
mail_log.save()Likely invalid or redundant comment.
| def login_imap(self): | ||
| if self.mailbox: | ||
| return | ||
| email_account = self.config['email_account'] | ||
| email_password = self.config['email_password'] | ||
| imap_server = self.config.get('imap_server', MailConstants.DEFAULT_IMAP_SERVER) | ||
| self.mailbox = MailBox(imap_server).login(email_account, email_password) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement context managers for IMAP and SMTP connections
Use context managers to ensure proper resource cleanup and connection handling.
+ def __enter__(self):
+ self.login_smtp()
+ return self
+
+ def __exit__(self, exc_type, exc_val, exc_tb):
+ self.logout_smtp()
+
def login_imap(self):
if self.mailbox:
return
email_account = self.config['email_account']
email_password = self.config['email_password']
imap_server = self.config.get('imap_server', MailConstants.DEFAULT_IMAP_SERVER)
- self.mailbox = MailBox(imap_server).login(email_account, email_password)
+ try:
+ self.mailbox = MailBox(imap_server).login(email_account, email_password)
+ except Exception as e:
+ logger.error(f"Failed to login to IMAP server: {str(e)}")
+ raise AppException("IMAP login failed") from eAlso applies to: 65-76
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
kairon/shared/channels/mail/processor.py (1)
21-298: Consider splitting mail processing responsibilitiesThe
MailProcessorclass currently handles both IMAP and SMTP operations. Consider splitting these into separate handler classes for better separation of concerns and maintainability:
ImapHandler: For reading and processing incoming emailsSmtpHandler: For sending outgoing emailsMailProcessor: For orchestrating the handlers and implementing business logicThis separation would:
- Improve maintainability and testability
- Allow for independent scaling of read and send operations
- Make it easier to implement retry mechanisms and circuit breakers
🧰 Tools
🪛 Ruff (0.8.0)
50-50: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
165-165: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
219-219: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
kairon/shared/channels/mail/processor.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
kairon/shared/channels/mail/processor.py
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
165-165: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
219-219: 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 (3)
kairon/shared/channels/mail/processor.py (3)
43-50:
Improve error handling with exception chaining
The error handling should maintain the original exception context for better debugging.
try:
state = MailChannelStateData.objects(bot=bot).first()
if not state:
state = MailChannelStateData(bot=bot)
state.save()
return state
except Exception as e:
- raise AppException(str(e))
+ raise AppException(f"Failed to get mail channel state data: {str(e)}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.0)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-59: 🛠️ Refactor suggestion
Add timeout and SSL configuration for IMAP connection
The IMAP connection should have timeout and SSL configuration options for security and reliability.
def login_imap(self):
if self.mailbox:
return
email_account = self.config['email_account']
email_password = self.config['email_password']
imap_server = self.config.get('imap_server', MailConstants.DEFAULT_IMAP_SERVER)
- self.mailbox = MailBox(imap_server).login(email_account, email_password)
+ timeout = self.config.get('imap_timeout', 30)
+ ssl = self.config.get('imap_ssl', True)
+ self.mailbox = MailBox(imap_server, timeout=timeout, ssl=ssl).login(email_account, email_password)Likely invalid or redundant comment.
105-125:
Enhance email sending security and reliability
The send_mail method needs input validation, specific exception handling, and rate limiting.
async def send_mail(self, to: str, subject: str, body: str, log_id: str):
exception = None
try:
+ if not to or '@' not in to:
+ raise ValueError(f"Invalid recipient email address: {to}")
+ if not subject or len(subject.strip()) == 0:
+ raise ValueError("Subject cannot be empty")
if body and len(body) > 0:
email_account = self.config['email_account']
msg = MIMEMultipart()
msg['From'] = email_account
msg['To'] = to
msg['Subject'] = subject
msg.attach(MIMEText(body, 'html'))
self.smtp.sendmail(email_account, to, msg.as_string())
- except Exception as e:
+ except smtplib.SMTPException as e:
logger.error(f"Error sending mail to {to}: {str(e)}")
exception = str(e)
+ except ValueError as e:
+ logger.error(f"Invalid input: {str(e)}")
+ exception = str(e)
+ except Exception as e:
+ logger.error(f"Unexpected error sending mail to {to}: {str(e)}")
+ exception = str(e)Likely invalid or redundant comment.
| def validate_smtp_connection(bot): | ||
| try: | ||
| mp = MailProcessor(bot) | ||
| mp.login_smtp() | ||
| mp.logout_smtp() | ||
| return True | ||
| except Exception as e: | ||
| logger.error(str(e)) | ||
| return False | ||
|
|
||
| @staticmethod | ||
| def validate_imap_connection(bot): | ||
| try: | ||
| mp = MailProcessor(bot) | ||
| mp.login_imap() | ||
| mp.logout_imap() | ||
| return True | ||
| except Exception as e: | ||
| logger.error(str(e)) | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle specific exceptions in connection validation
The validation methods should handle specific exceptions to provide better error information.
@staticmethod
def validate_smtp_connection(bot):
try:
mp = MailProcessor(bot)
mp.login_smtp()
mp.logout_smtp()
return True
- except Exception as e:
- logger.error(str(e))
+ except smtplib.SMTPAuthenticationError as e:
+ logger.error(f"SMTP authentication failed: {str(e)}")
+ return False
+ except smtplib.SMTPException as e:
+ logger.error(f"SMTP connection error: {str(e)}")
+ return False
+ except Exception as e:
+ logger.error(f"Unexpected error in SMTP validation: {str(e)}")
return FalseCommittable suggestion skipped: line range outside the PR's diff.
| def get_log(bot_id: str, offset: int, limit: int) -> dict: | ||
| """ | ||
| Get logs for a bot | ||
| """ | ||
| try: | ||
| count = MailResponseLog.objects(bot=bot_id).count() | ||
| logs = MailResponseLog.objects(bot=bot_id).order_by('-timestamp').skip(offset).limit(limit) | ||
| result = [] | ||
| for log in logs: | ||
| log = log.to_mongo().to_dict() | ||
| log.pop('_id') | ||
| log.pop('bot') | ||
| log.pop('user') | ||
| result.append(log) | ||
| return { | ||
| "logs": result, | ||
| "count": count | ||
| } | ||
| except Exception as e: | ||
| raise AppException(str(e)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling in log retrieval
The log retrieval method needs input validation and proper exception chaining.
@staticmethod
def get_log(bot_id: str, offset: int, limit: int) -> dict:
+ if offset < 0:
+ raise ValueError("Offset must be non-negative")
+ if limit <= 0:
+ raise ValueError("Limit must be positive")
try:
count = MailResponseLog.objects(bot=bot_id).count()
logs = MailResponseLog.objects(bot=bot_id).order_by('-timestamp').skip(offset).limit(limit)
result = []
for log in logs:
log = log.to_mongo().to_dict()
log.pop('_id')
log.pop('bot')
log.pop('user')
result.append(log)
return {
"logs": result,
"count": count
}
except Exception as e:
- raise AppException(str(e))
+ raise AppException(f"Failed to retrieve mail logs: {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.
| def get_log(bot_id: str, offset: int, limit: int) -> dict: | |
| """ | |
| Get logs for a bot | |
| """ | |
| try: | |
| count = MailResponseLog.objects(bot=bot_id).count() | |
| logs = MailResponseLog.objects(bot=bot_id).order_by('-timestamp').skip(offset).limit(limit) | |
| result = [] | |
| for log in logs: | |
| log = log.to_mongo().to_dict() | |
| log.pop('_id') | |
| log.pop('bot') | |
| log.pop('user') | |
| result.append(log) | |
| return { | |
| "logs": result, | |
| "count": count | |
| } | |
| except Exception as e: | |
| raise AppException(str(e)) | |
| def get_log(bot_id: str, offset: int, limit: int) -> dict: | |
| """ | |
| Get logs for a bot | |
| """ | |
| if offset < 0: | |
| raise ValueError("Offset must be non-negative") | |
| if limit <= 0: | |
| raise ValueError("Limit must be positive") | |
| try: | |
| count = MailResponseLog.objects(bot=bot_id).count() | |
| logs = MailResponseLog.objects(bot=bot_id).order_by('-timestamp').skip(offset).limit(limit) | |
| result = [] | |
| for log in logs: | |
| log = log.to_mongo().to_dict() | |
| log.pop('_id') | |
| log.pop('bot') | |
| log.pop('user') | |
| result.append(log) | |
| return { | |
| "logs": result, | |
| "count": count | |
| } | |
| except Exception as e: | |
| raise AppException(f"Failed to retrieve mail logs: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.8.0)
165-165: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| def __init__(self, bot): | ||
| self.bot = bot | ||
| self.config = ChatDataProcessor.get_channel_config(ChannelTypes.MAIL, bot, False)['config'] | ||
| self.intent = self.config.get('intent') | ||
| self.mail_template = self.config.get('mail_template', MailConstants.DEFAULT_TEMPLATE) | ||
| self.bot_settings = BotSettings.objects(bot=self.bot).get() | ||
| self.state = MailProcessor.get_mail_channel_state_data(bot) | ||
| bot_info = Bot.objects.get(id=bot) | ||
| self.account = bot_info.account | ||
| self.mailbox = None | ||
| self.smtp = None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for required configuration parameters
The initialization should validate the presence and format of required configuration parameters, especially for sensitive credentials.
def __init__(self, bot):
self.bot = bot
self.config = ChatDataProcessor.get_channel_config(ChannelTypes.MAIL, bot, False)['config']
+ required_params = ['email_account', 'email_password']
+ if not all(param in self.config for param in required_params):
+ raise AppException("Missing required mail configuration parameters")
+ if not self.config['email_account'] or not '@' in self.config['email_account']:
+ raise AppException("Invalid email account format")
self.intent = self.config.get('intent')
self.mail_template = self.config.get('mail_template', MailConstants.DEFAULT_TEMPLATE)
self.bot_settings = BotSettings.objects(bot=self.bot).get()📝 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 __init__(self, bot): | |
| self.bot = bot | |
| self.config = ChatDataProcessor.get_channel_config(ChannelTypes.MAIL, bot, False)['config'] | |
| self.intent = self.config.get('intent') | |
| self.mail_template = self.config.get('mail_template', MailConstants.DEFAULT_TEMPLATE) | |
| self.bot_settings = BotSettings.objects(bot=self.bot).get() | |
| self.state = MailProcessor.get_mail_channel_state_data(bot) | |
| bot_info = Bot.objects.get(id=bot) | |
| self.account = bot_info.account | |
| self.mailbox = None | |
| self.smtp = None | |
| def __init__(self, bot): | |
| self.bot = bot | |
| self.config = ChatDataProcessor.get_channel_config(ChannelTypes.MAIL, bot, False)['config'] | |
| required_params = ['email_account', 'email_password'] | |
| if not all(param in self.config for param in required_params): | |
| raise AppException("Missing required mail configuration parameters") | |
| if not self.config['email_account'] or not '@' in self.config['email_account']: | |
| raise AppException("Invalid email account format") | |
| self.intent = self.config.get('intent') | |
| self.mail_template = self.config.get('mail_template', MailConstants.DEFAULT_TEMPLATE) | |
| self.bot_settings = BotSettings.objects(bot=self.bot).get() | |
| self.state = MailProcessor.get_mail_channel_state_data(bot) | |
| bot_info = Bot.objects.get(id=bot) | |
| self.account = bot_info.account | |
| self.mailbox = None | |
| self.smtp = None |
| async def process_messages(bot: str, batch: [dict]): | ||
| """ | ||
| Pass messages to bot and send responses | ||
| """ | ||
| try: | ||
| from kairon.chat.utils import ChatUtils | ||
| mp = MailProcessor(bot) | ||
| user_messages: [str] = [] | ||
| responses = [] | ||
| for mail in batch: | ||
| try: | ||
| entities = { | ||
| 'mail_id': mail['mail_id'], | ||
| 'subject': mail['subject'], | ||
| 'date': mail['date'], | ||
| 'body': mail['body'] | ||
| } | ||
| entities_str = ', '.join([f'"{key}": "{value}"' for key, value in entities.items() if value and value != 'null']) | ||
| user_msg = f'/{mp.intent}{{{entities_str}}}' | ||
| user_messages.append(user_msg) | ||
| subject = mail.get('subject', 'Reply') | ||
| if not subject.startswith('Re:'): | ||
| subject = f"Re: {subject}" | ||
|
|
||
| responses.append({ | ||
| 'to': mail['mail_id'], | ||
| 'subject': subject, | ||
| 'body': '', | ||
| 'log_id': mail['log_id'] | ||
| }) | ||
| except Exception as e: | ||
| logger.error(str(e)) | ||
|
|
||
| chat_responses = await ChatUtils.process_messages_via_bot(user_messages, | ||
| mp.account, | ||
| bot, | ||
| mp.bot_settings.user, | ||
| False, | ||
| { | ||
| 'channel': ChannelTypes.MAIL.value | ||
| }) | ||
|
|
||
| for index, response in enumerate(chat_responses): | ||
| responses[index]['body'] = mp.process_mail(response, log_id=batch[index]['log_id']) | ||
|
|
||
| mp.login_smtp() | ||
| tasks = [mp.send_mail(**response) for response in responses] | ||
| await asyncio.gather(*tasks) | ||
| mp.logout_smtp() | ||
|
|
||
| except Exception as e: | ||
| raise AppException(str(e)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement batch processing safeguards
The message processing needs batch size limits and partial failure handling.
@staticmethod
async def process_messages(bot: str, batch: [dict]):
+ MAX_BATCH_SIZE = 50
try:
+ if len(batch) > MAX_BATCH_SIZE:
+ raise ValueError(f"Batch size exceeds maximum limit of {MAX_BATCH_SIZE}")
from kairon.chat.utils import ChatUtils
mp = MailProcessor(bot)
user_messages: [str] = []
responses = []
+ failed_messages = []
for mail in batch:
try:
entities = {
'mail_id': mail['mail_id'],
'subject': mail['subject'],
'date': mail['date'],
'body': mail['body']
}
entities_str = ', '.join([f'"{key}": "{value}"' for key, value in entities.items() if value and value != 'null'])
user_msg = f'/{mp.intent}{{{entities_str}}}'
user_messages.append(user_msg)
subject = mail.get('subject', 'Reply')
if not subject.startswith('Re:'):
subject = f"Re: {subject}"
responses.append({
'to': mail['mail_id'],
'subject': subject,
'body': '',
'log_id': mail['log_id']
})
except Exception as e:
logger.error(f"Failed to process message: {str(e)}")
+ failed_messages.append(mail)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| def read_mails(bot: str) -> ([dict], str, int): | ||
| """ | ||
| Read mails from the mailbox | ||
| Parameters: | ||
| - bot: str - bot id | ||
| Returns: | ||
| - list of messages - each message is a dict with the following | ||
| - mail_id | ||
| - subject | ||
| - date | ||
| - body | ||
| - log_id | ||
| - user | ||
| - time_shift | ||
|
|
||
| """ | ||
| logger.info(f"reading mails for {bot}") | ||
| mp = MailProcessor(bot) | ||
| time_shift = int(mp.config.get('interval', 20 * 60)) | ||
| last_read_timestamp = datetime.now() - timedelta(seconds=time_shift) | ||
| messages = [] | ||
| is_logged_in = False | ||
| last_processed_uid = mp.state.last_email_uid | ||
| query = f'{int(last_processed_uid) + 1}:*' | ||
| try: | ||
| mp.login_imap() | ||
| is_logged_in = True | ||
| msgs = mp.mailbox.fetch(AND(date_gte=last_read_timestamp.date(), uid=query), mark_seen=False) | ||
| for msg in msgs: | ||
| if int(msg.uid) <= last_processed_uid: | ||
| continue | ||
| last_processed_uid = int(msg.uid) | ||
| subject = msg.subject | ||
| sender_id = msg.from_ | ||
| date = msg.date | ||
| body = msg.text or msg.html or "" | ||
| #attachments = msg.attachments | ||
| mail_log = MailResponseLog(sender_id = sender_id, | ||
| subject = subject, | ||
| body = body, | ||
| bot = bot, | ||
| user = mp.bot_settings.user, | ||
| status=MailStatus.Processing.value, | ||
| timestamp = time.time()) | ||
| mail_log.save() | ||
| message_entry = { | ||
| 'mail_id': sender_id, | ||
| 'subject': subject, | ||
| 'date': str(date), | ||
| 'body': body, | ||
| 'log_id': str(mail_log.id) | ||
| } | ||
| messages.append(message_entry) | ||
| mp.logout_imap() | ||
|
|
||
| mp.state.last_email_uid = last_processed_uid | ||
| mp.state.save() | ||
|
|
||
| is_logged_in = False | ||
| return messages, mp.bot_settings.user, time_shift | ||
| except Exception as e: | ||
| logger.exception(e) | ||
| if is_logged_in: | ||
| mp.logout_imap() | ||
| return [], mp.bot_settings.user, time_shift | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Implement email fetch limits and add attachment field to MailResponseLog
The review comment is valid and needs additional improvements:
- The fetch limit suggestion is correct as there's currently no limit on email fetching
- The MailResponseLog model doesn't have an attachments field to store email attachments
- Generic exception handling should be more specific
Required changes:
- Add
MAX_FETCH_LIMITconstant and use it withfetch()method - Add
attachmentsfield toMailResponseLogmodel indata_objects.py - Process and store email attachments instead of commenting them out
- Split exception handling into specific cases (connection errors, authentication errors)
🔗 Analysis chain
Implement email fetch limits and handle attachments
The mail reading method needs fetch limits and proper attachment handling.
@staticmethod
def read_mails(bot: str) -> ([dict], str, int):
+ MAX_FETCH_LIMIT = 100
logger.info(f"reading mails for {bot}")
mp = MailProcessor(bot)
time_shift = int(mp.config.get('interval', 20 * 60))
last_read_timestamp = datetime.now() - timedelta(seconds=time_shift)
messages = []
is_logged_in = False
last_processed_uid = mp.state.last_email_uid
query = f'{int(last_processed_uid) + 1}:*'
try:
mp.login_imap()
is_logged_in = True
- msgs = mp.mailbox.fetch(AND(date_gte=last_read_timestamp.date(), uid=query), mark_seen=False)
+ msgs = mp.mailbox.fetch(AND(date_gte=last_read_timestamp.date(), uid=query), mark_seen=False, limit=MAX_FETCH_LIMIT)
for msg in msgs:
if int(msg.uid) <= last_processed_uid:
continue
last_processed_uid = int(msg.uid)
subject = msg.subject
sender_id = msg.from_
date = msg.date
body = msg.text or msg.html or ""
- #attachments = msg.attachments
+ # TODO: Handle attachments
+ # attachments = msg.attachmentsLet's verify the attachment handling implementation in other files:
Let's check the implementation details of attachment handling in the mail processor:
Let's check the MailResponseLog model to see if it supports attachment storage:
Let's try to find the MailResponseLog definition using ripgrep:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for attachment handling in related files
rg -l "attachment" --type py
Length of output: 1015
Script:
#!/bin/bash
# Search for attachment handling in mail processor
rg -A 10 "attachment" kairon/shared/channels/mail/processor.py
Length of output: 680
Script:
#!/bin/bash
# Search for MailResponseLog model definition
ast-grep --pattern 'class MailResponseLog'
Length of output: 44
Script:
#!/bin/bash
# Search for MailResponseLog definition and usage
rg -A 10 "class MailResponseLog" --type py
Length of output: 822
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation