Mail channel availability verification #1727
Conversation
WalkthroughThe pull request introduces new functionality for checking email configurations across multiple files in the Kairon project. A new endpoint is added to the FastAPI router to verify email existence, accompanied by a corresponding method in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/integration_test/services_test.py (3)
24282-24296: Enhance test setup with best practices.Consider the following improvements:
- Add a docstring explaining the test purpose and scenarios
- Move test configuration to fixtures for reusability
- Use environment variables or test constants for sensitive data
@patch('kairon.shared.chat.processor.ChatDataProcessor.get_all_channel_configs') def test_check_mail_exists(mock_get_all_channel_configs): + """ + Test mail channel configuration existence verification: + 1. Verify existing email and subject combination + 2. Verify existing email with different subject + 3. Verify non-existing email + """ config_dict = { 'connector_type': 'mail', 'config': { 'email_account': '[email protected]', 'imap_server': 'imap.example.com', 'smtp_server': 'smtp.example.com', 'smtp_port': 587, - 'email_password': 'password', + 'email_password': pytest.email_test_password, # Use test constant 'subject': 'email channel test', } }
24297-24304: Improve test case readability.The test assertions are correct but could be more self-documenting:
+ # Case 1: Exact match of email and subject should prevent creation response = client.post( f"/api/bot/{pytest.bot}/channels/mail/exists", json=config_dict, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) - data = response.json() + response_data = response.json() - assert not data['data']['can_create'] - assert data['data']['email_exists'] + assert not response_data['data']['can_create'], "Should not allow creation for exact match" + assert response_data['data']['email_exists'], "Should indicate email exists"
24306-24325: Reduce code duplication and enhance assertions.The test case has duplicated configuration and missing status code validation:
- Create a helper function to generate test configs
- Add status code assertion
- config2 = { - 'connector_type': 'mail', - 'config': { - 'email_account': '[email protected]', - 'imap_server': 'imap.example.com', - 'smtp_server': 'smtp.example.com', - 'smtp_port': 587, - 'email_password': 'password', - 'subject': 'different subject', - } - } + config2 = create_mail_config( + email='[email protected]', + subject='different subject' + ) response = client.post( f"/api/bot/{pytest.bot}/channels/mail/exists", json=config2, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) + assert response.status_code == 200, "Should return OK status" data = response.json() assert data['data']['can_create'] assert data['data']['email_exists']Helper function to add:
def create_mail_config(email: str, subject: str) -> dict: """Generate mail configuration for testing.""" return { 'connector_type': 'mail', 'config': { 'email_account': email, 'imap_server': 'imap.example.com', 'smtp_server': 'smtp.example.com', 'smtp_port': 587, 'email_password': pytest.email_test_password, 'subject': subject, } }kairon/shared/chat/processor.py (1)
111-124: LGTM! Consider adding return type hint and improving docstring.The implementation is clean and memory-efficient using yield. However, the docstring could be more descriptive.
@staticmethod - def get_all_channel_configs(connector_type: str, mask_characters: bool = True, **kwargs): + def get_all_channel_configs(connector_type: str, mask_characters: bool = True, **kwargs) -> dict: """ fetch all channel configs for bot :param connector_type: channel name :param mask_characters: whether to mask the security keys default is True + :param kwargs: additional filter parameters :return: Generator yielding channel configuration dictionaries + :rtype: Generator[dict, None, None] """kairon/shared/channels/mail/processor.py (1)
58-73: Optimize the email configuration check implementation.The logic is correct but can be made more efficient and readable.
Consider this optimized implementation:
@staticmethod def check_email_config_exists(config_dict: dict) -> dict: """ Check if an email configuration exists Args: config_dict: Email configuration dictionary Returns: dict: Status indicating if configuration can be created and if email exists """ email_account = config_dict['email_account'] for config in ChatDataProcessor.get_all_channel_configs(ChannelTypes.MAIL, False): if config['config']['email_account'] != email_account: continue - is_same = True - for key in config_dict: - if key != 'mail_template' and config['config'].get(key) != config_dict[key]: - is_same = False - break - if is_same: + config_keys = set(config_dict.keys()) - {'mail_template'} + if all(config['config'].get(key) == config_dict[key] for key in config_keys): return {'can_create': False, 'email_exists': True} - else: - return {'can_create': True, 'email_exists': True} + return {'can_create': True, 'email_exists': True} return {'can_create': True, 'email_exists': False}tests/unit_test/chat/chat_test.py (1)
940-950: LGTM! Consider enhancing test coverage.The test effectively verifies the basic functionality of
get_all_channel_configs. However, consider adding:
- A docstring explaining the test purpose
- Assertions for other configuration fields
- Error case scenarios (e.g., non-existent connector type)
def test_get_all_channel_configs(self): + """ + Test retrieval of all channel configurations for telegram connector type. + Verifies both masked and unmasked configurations. + """ configs = list(ChatDataProcessor.get_all_channel_configs('telegram')) assert len(configs) == 1 assert configs[0].get("connector_type") == "telegram" assert str(configs[0]["config"].get("access_token")).__contains__("***") + assert configs[0]["config"].get("webhook_url") is not None + assert configs[0]["config"].get("username_for_bot") is not None configs = list(ChatDataProcessor.get_all_channel_configs('telegram', mask_characters=False)) assert len(configs) == 1 assert configs[0].get("connector_type") == "telegram" assert not str(configs[0]["config"].get("access_token")).__contains__("***") + assert configs[0]["config"].get("webhook_url") is not None + assert configs[0]["config"].get("username_for_bot") is not None + + # Test error case + configs = list(ChatDataProcessor.get_all_channel_configs('non_existent_type')) + assert len(configs) == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
kairon/api/app/routers/bot/channels.py(2 hunks)kairon/shared/channels/mail/processor.py(1 hunks)kairon/shared/chat/processor.py(1 hunks)tests/integration_test/services_test.py(1 hunks)tests/unit_test/channels/mail_channel_test.py(1 hunks)tests/unit_test/chat/chat_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/api/app/routers/bot/channels.py
39-39: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
🔇 Additional comments (2)
tests/integration_test/services_test.py (1)
24297-24299: Verify the endpoint implementation.The test is calling an endpoint that should be implemented in the FastAPI router.
Let's verify the endpoint implementation:
Also applies to: 24318-24320, 24339-24341
✅ Verification successful
Endpoint implementation verified
The endpoint
/api/bot/{bot}/channels/mail/existsis properly implemented inkairon/api/app/routers/bot/channels.pyas a POST endpoint with thecheck_mail_existshandler.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the endpoint is properly implemented in the FastAPI router # Look for the endpoint definition rg -A 5 "channels/mail/exists" --type py # Look for the MailProcessor method ast-grep --pattern 'def check_email_config_exists($_)'Length of output: 1521
Script:
#!/bin/bash # Search for mail-related router files and endpoint definitions # Look for router files related to mail fd -e py | rg -i "mail|email" # Search for FastAPI route decorators with mail/email rg -B 2 -A 5 "@(router|app)\.(get|post|put|delete).+mail" --type py # Search for FastAPI route decorators with channels rg -B 2 -A 5 "@(router|app)\.(get|post|put|delete).+channels" --type pyLength of output: 5488
tests/unit_test/channels/mail_channel_test.py (1)
591-625: LGTM! Comprehensive test coverage.The test implementation is well-structured and covers all important scenarios:
- No existing configuration
- Identical configuration exists
- Different configuration with same email exists
| config3 = { | ||
| 'connector_type': 'mail', | ||
| 'config': { | ||
| 'email_account': '[email protected]', | ||
| 'imap_server': 'imap.example.com', | ||
| 'smtp_server': 'smtp.example.com', | ||
| 'smtp_port': 587, | ||
| 'email_password': 'password', | ||
| 'subject': 'different subject', | ||
| } | ||
| } | ||
|
|
||
| response = client.post( | ||
| f"/api/bot/{pytest.bot}/channels/mail/exists", | ||
| json=config3, | ||
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | ||
| ) | ||
| data = response.json() | ||
| assert data['data']['can_create'] | ||
| assert not data['data']['email_exists'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error scenarios and reduce duplication.
The test suite needs additional coverage:
- Use the suggested helper function for config3
- Add test cases for:
- Invalid email format
- Missing required fields
- Invalid SMTP/IMAP server configurations
Example additional test cases:
@pytest.mark.parametrize("invalid_config,expected_error", [
(
{"config": {"email_account": "invalid-email"}},
"Invalid email format"
),
(
{"config": {"email_account": "[email protected]"}},
"Missing required fields"
),
(
{
"config": {
"email_account": "[email protected]",
"smtp_server": "invalid-server"
}
},
"Invalid SMTP configuration"
)
])
def test_check_mail_exists_errors(invalid_config, expected_error):
response = client.post(
f"/api/bot/{pytest.bot}/channels/mail/exists",
json=invalid_config,
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
assert response.status_code == 422
assert expected_error in response.json()["message"]| @router.post('/mail/exists', response_model=Response) | ||
| async def check_mail_exists( | ||
| request_data: ChannelRequest, | ||
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=DESIGNER_ACCESS) | ||
| ): | ||
| """ | ||
| Check if email exists in the system | ||
| """ | ||
| return Response(data=MailProcessor.check_email_config_exists(request_data.dict()['config'])) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Security function call in argument defaults and improve docstring.
The endpoint implementation is good, but there are two improvements needed:
- Move the Security function call inside the function to avoid potential issues with argument defaults:
@router.post('/mail/exists', response_model=Response)
async def check_mail_exists(
request_data: ChannelRequest,
- current_user: User = Security(Authentication.get_current_user_and_bot, scopes=DESIGNER_ACCESS)
+ current_user: User = None
):
+ if current_user is None:
+ current_user = Security(Authentication.get_current_user_and_bot, scopes=DESIGNER_ACCESS)- Enhance the docstring to better describe the endpoint:
"""
- Check if email exists in the system
+ Check if an email configuration already exists in the system.
+
+ Args:
+ request_data: Channel configuration request containing email details
+ current_user: Authenticated user with designer access
+
+ Returns:
+ Response containing email existence check results
"""Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
39-39: 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit_test/channels/mail_channel_test.py (5)
592-598: Consider expanding the fixture with additional configuration fields.The fixture could be more comprehensive by including other possible configuration fields like
imap_server,smtp_server,smtp_port, etc., to ensure thorough test coverage.@pytest.fixture def config_dict(self): return { 'email_account': '[email protected]', - 'subjects': 'subject1,subject2' + 'subjects': 'subject1,subject2', + 'imap_server': 'imap.example.com', + 'smtp_server': 'smtp.example.com', + 'smtp_port': 587, + 'email_password': 'password' }
599-604: Follow Python's idiomatic boolean comparisons.Replace the explicit comparison with
Falsewith a more Pythonic expression.@patch('kairon.shared.chat.processor.ChatDataProcessor.get_all_channel_configs') def test_check_email_config_exists_no_existing_config(self,mock_get_all_channel_configs, config_dict): mock_get_all_channel_configs.return_value = [] result = MailProcessor.check_email_config_exists(config_dict) - assert result == False + assert not result🧰 Tools
🪛 Ruff (0.8.2)
603-603: Avoid equality comparisons to
False; useif not result:for false checksReplace with
not result(E712)
605-612: Follow Python's idiomatic boolean comparisons.Replace the explicit comparison with
Truewith a more Pythonic expression.@patch('kairon.shared.chat.processor.ChatDataProcessor.get_all_channel_configs') def test_check_email_config_exists_same_config_exists(self, mock_get_all_channel_configs, config_dict): mock_get_all_channel_configs.return_value = [{ 'config': config_dict }] result = MailProcessor.check_email_config_exists(config_dict) - assert result == True + assert result🧰 Tools
🪛 Ruff (0.8.2)
611-611: Avoid equality comparisons to
True; useif result:for truth checksReplace with
result(E712)
613-622: Follow Python's idiomatic boolean comparisons and verify config immutability.
- Replace the explicit comparison with
Falsewith a more Pythonic expression.- Add an assertion to verify that the original
config_dictwasn't modified.@patch('kairon.shared.chat.processor.ChatDataProcessor.get_all_channel_configs') def test_check_email_config_exists_different_config_exists(self,mock_get_all_channel_configs, config_dict): existing_config = config_dict.copy() existing_config['subjects'] = 'subject3' + original_subjects = config_dict['subjects'] # Store original value mock_get_all_channel_configs.return_value = [{ 'config': existing_config }] result = MailProcessor.check_email_config_exists(config_dict) - assert result == False + assert not result + assert config_dict['subjects'] == original_subjects # Verify config_dict wasn't modified🧰 Tools
🪛 Ruff (0.8.2)
621-621: Avoid equality comparisons to
False; useif not result:for false checksReplace with
not result(E712)
623-631: Enhance test coverage and follow Python's idiomatic boolean comparisons.
- Replace the explicit comparison with
Truewith a more Pythonic expression.- Consider adding test cases for edge cases.
@patch('kairon.shared.chat.processor.ChatDataProcessor.get_all_channel_configs') def test_check_email_config_exists_partial_subject_match(self, mock_get_all_channel_configs, config_dict): existing_config = config_dict.copy() existing_config['subjects'] = 'subject1,subject3' mock_get_all_channel_configs.return_value = [{ 'config': existing_config }] result = MailProcessor.check_email_config_exists(config_dict) - assert result == True + assert resultConsider adding these test cases:
def test_check_email_config_exists_empty_subjects(self, mock_get_all_channel_configs, config_dict): """Test when subjects are empty strings""" config_dict['subjects'] = '' existing_config = config_dict.copy() existing_config['subjects'] = '' mock_get_all_channel_configs.return_value = [{'config': existing_config}] result = MailProcessor.check_email_config_exists(config_dict) assert result def test_check_email_config_exists_whitespace_subjects(self, mock_get_all_channel_configs, config_dict): """Test when subjects contain whitespace""" config_dict['subjects'] = 'subject1, subject2' # Note the space after comma existing_config = config_dict.copy() existing_config['subjects'] = 'subject1,subject2' # No space mock_get_all_channel_configs.return_value = [{'config': existing_config}] result = MailProcessor.check_email_config_exists(config_dict) assert result🧰 Tools
🪛 Ruff (0.8.2)
631-631: Avoid equality comparisons to
True; useif result:for truth checksReplace with
result(E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
kairon/api/app/routers/bot/channels.py(0 hunks)kairon/shared/channels/mail/processor.py(1 hunks)kairon/shared/chat/processor.py(2 hunks)tests/integration_test/services_test.py(0 hunks)tests/unit_test/channels/mail_channel_test.py(1 hunks)tests/unit_test/chat/chat_test.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/integration_test/services_test.py
- kairon/api/app/routers/bot/channels.py
🚧 Files skipped from review as they are similar to previous changes (3)
- kairon/shared/chat/processor.py
- tests/unit_test/chat/chat_test.py
- kairon/shared/channels/mail/processor.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/channels/mail_channel_test.py
603-603: Avoid equality comparisons to False; use if not result: for false checks
Replace with not result
(E712)
611-611: Avoid equality comparisons to True; use if result: for truth checks
Replace with result
(E712)
621-621: Avoid equality comparisons to False; use if not result: for false checks
Replace with not result
(E712)
631-631: Avoid equality comparisons to True; use if result: for truth checks
Replace with result
(E712)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests