Name Validation #1803
Name Validation #1803
Conversation
…ases for the same.
WalkthroughThis pull request introduces additional input validations and more specific error handling across two main modules. In the account management module, validations for bot names were added in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
tests/unit_test/data_processor/data_processor2_test.py (13)
400-416: Consider adding more test cases for HTTP config name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_update_http_config_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "action_name": "http-action", "response": {"value": "string"}, "http_url": "http://www.google.com", "request_method": "GET", "http_params_list": [ {"key": "testParam1", "parameter_type": "value", "value": "testValue1"} ], } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.update_http_config(config, user, bot) + + # Test more invalid characters + config["action_name"] = "http@action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_http_config(config, user, bot) + + config["action_name"] = "http.action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_http_config(config, user, bot)
434-448: Consider adding more test cases for slot set action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_slot_set_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "action-set-name-slot", "set_slots": [ {"name": "name", "type": "from_value", "value": 5}, {"name": "age", "type": "reset_slot"}, ], } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_slot_set_action(config, user, bot) + + # Test more invalid characters + config["name"] = "action@set@name@slot" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_slot_set_action(config, user, bot) + + config["name"] = "action.set.name.slot" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_slot_set_action(config, user, bot)
471-490: Consider adding more test cases for email action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_email_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "action_name": "email~config", "smtp_url": "test.test.com", "smtp_port": 25, "smtp_userid": None, "smtp_password": {"value": "test"}, "from_email": {"value": "from_email", "parameter_type": "slot"}, "to_email": {"value": ["test@test.com", "test1@test.com"], "parameter_type": "value"}, "subject": "Test Subject", "response": "Test Response", "tls": False, } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_email_action(config, user, bot) + + # Test more invalid characters + config["action_name"] = "email@config" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_email_action(config, user, bot) + + config["action_name"] = "email.config" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_email_action(config, user, bot)
508-522: Consider adding more test cases for Google search action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_google_search_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "google>custom<search", "api_key": {"value": "12345678"}, "search_engine_id": "asdfg:123456", "failure_response": "I have failed to process your request", "website": "https://www.google.com", } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_google_search_action(config, user, bot) + + # Test more invalid characters + config["name"] = "google@custom@search" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_google_search_action(config, user, bot) + + config["name"] = "google.custom.search" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_google_search_action(config, user, bot)
544-562: Consider adding more test cases for Jira action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_jira_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' url = "https://test_add_jira_action_invalid_config.net" config = { "name": "jira'action'new", "url": url, "user_name": "test@digite.com", "api_token": {"value": "ASDFGHJKL"}, "project_key": "HEL", "issue_type": "Bug", "summary": "new user", "response": "We have logged a ticket", } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_jira_action(config, user, bot) + + # Test more invalid characters + config["name"] = "jira@action@new" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_jira_action(config, user, bot) + + config["name"] = "jira.action.new" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_jira_action(config, user, bot)
581-596: Consider adding more test cases for Zendesk action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_zendesk_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "zendesk@action", "subdomain": "digite751", "api_token": {"value": "123456789"}, "subject": "new ticket", "user_name": "udit.pandey@digite.com", "response": "ticket filed", } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_zendesk_action(config, user, bot) + + # Test more invalid characters + config["name"] = "zendesk-action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_zendesk_action(config, user, bot) + + config["name"] = "zendesk.action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_zendesk_action(config, user, bot)
620-640: Consider adding more test cases for Pipedrive action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_pipedrive_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "pipedrive#leads", "domain": "https://digite751.pipedrive.com/", "api_token": {"value": "12345678"}, "title": "new lead", "response": "I have failed to create lead for you", "metadata": { "name": "name", "org_name": "organization", "email": "email", "phone": "phone", }, } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_pipedrive_action(config, user, bot) + + # Test more invalid characters + config["name"] = "pipedrive@leads" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_pipedrive_action(config, user, bot) + + config["name"] = "pipedrive.leads" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_pipedrive_action(config, user, bot)
661-678: Consider adding more test cases for Hubspot forms action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_hubspot_forms_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "action(hubspot)forms", "portal_id": "12345678", "form_guid": "asdfg:123456", "fields": [ {"key": "email", "value": "email_slot", "parameter_type": "slot"}, {"key": "firstname", "value": "firstname_slot", "parameter_type": "slot"}, ], "response": "Form submitted", } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_hubspot_forms_action(config, user, bot) + + # Test more invalid characters + config["name"] = "action@hubspot@forms" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_hubspot_forms_action(config, user, bot) + + config["name"] = "action.hubspot.forms" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_hubspot_forms_action(config, user, bot)
704-726: Consider adding more test cases for Razorpay action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_razorpay_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' action_name = "razorpay`action" config = { "name": action_name, "api_key": {"value": "API_KEY", "parameter_type": "key_vault"}, "api_secret": {"value": "API_SECRET", "parameter_type": "key_vault"}, "amount": {"value": "amount", "parameter_type": "slot"}, "currency": {"value": "INR", "parameter_type": "value"}, "username": {"parameter_type": "sender_id"}, "email": {"parameter_type": "sender_id"}, "contact": {"value": "contact", "parameter_type": "slot"}, "notes": [ {"key": "order_id", "parameter_type": "slot", "value": "order_id"}, {"key": "phone_number", "parameter_type": "value", "value": "9876543210"} ] } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_razorpay_action(config, user, bot) + + # Test more invalid characters + config["name"] = "razorpay@action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_razorpay_action(config, user, bot) + + config["name"] = "razorpay.action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_razorpay_action(config, user, bot)
743-756: Consider adding more test cases for Python script action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_update_pyscript_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' script = "bot_response='hello world'" config = { "name": "pyscript-action", "source_code": script, "dispatch_response": False, } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.update_pyscript_action(config, user, bot) + + # Test more invalid characters + config["name"] = "pyscript@action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_pyscript_action(config, user, bot) + + config["name"] = "pyscript.action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_pyscript_action(config, user, bot)
777-794: Consider adding more test cases for database action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_update_db_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "add-vectordb-action", "collection": 'test_add_vectordb_action_empty_name', "payload": [{ "type": "from_value", "value": {"ids": [0], "with_payload": True, "with_vector": True}, "query_type": "embedding_search", }], "response": {"value": "0"}, } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.update_db_action(config, user, bot) + + # Test more invalid characters + config["name"] = "add@vectordb@action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_db_action(config, user, bot) + + config["name"] = "add.vectordb.action" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.update_db_action(config, user, bot)
811-824: Consider adding more test cases for callback action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_edit_callback_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": "callback@1", "pyscript_code": "bot_response = 'Hello World!'", "validation_secret": "string", "execution_mode": "async" } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.edit_callback_action(config, user, bot) + + # Test more invalid characters + config["name"] = "callback-1" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_callback_action(config, user, bot) + + config["name"] = "callback.1" + with pytest.raises(AppException, + match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): + processor.edit_callback_action(config, user, bot)
851-874: Consider adding more test cases for schedule action name validation.While the current test case is valid, consider adding more test cases with different invalid characters to match the coverage level of other validation tests.
def test_update_schedule_action_with_invalid_name(): processor = MongoProcessor() bot = 'test_bot' user = 'test_user' config = { "name": " test schedule action", "schedule_time": {"value": "2024-08-06T09:00:00.000+0530", "parameter_type": "value"}, "timezone": None, "schedule_action": "test_pyscript", "response_text": "action scheduled", "params_list": [ { "key": "param_key", "value": "param_1", "parameter_type": "value", "count": 0 } ], "dispatch_bot_response": True } with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): processor.update_schedule_ </blockquote></details> <details> <summary>tests/unit_test/api/api_processor_test.py (1)</summary><blockquote> `104-124`: **Remove unused variables and consider adding more test cases.** The test implementation looks good but has some minor issues: 1. The variables `account`, `user`, and `is_new_account` are assigned but never used in the test cases. 2. Consider adding more test cases for other special characters and edge cases. Apply this diff to remove unused variables and simplify the test: ```diff def test_add_bot_with_invalid_name(self): import re - account = pytest.account - user = "fshaikh@digite.com" - is_new_account = True with pytest.raises(AppException, match=re.escape("Invalid name! Only letters, numbers, and underscores (_) are allowed.")): name = "test#21" - AccountProcessor.add_bot(name=name, account=account, user=user, is_new_account=is_new_account) + AccountProcessor.add_bot(name=name, account=pytest.account, user="fshaikh@digite.com", is_new_account=True)kairon/shared/data/processor.py (1)
4134-4808: Overall: Well-implemented name validation changesThe changes consistently add name validation across all action types to ensure names only contain letters, numbers and underscores. The implementation:
- Uses a consistent validation method (Utility.special_match)
- Provides clear error messages
- Validates during both creation and updates
- Maintains backward compatibility
Consider adding these validations to the base Action class or a validation decorator to reduce code duplication in the future.
🧰 Tools
🪛 Ruff (0.8.2)
4157-4157: Local variable
responseis assigned to but never usedRemove assignment to unused variable
response(F841)
4276-4278: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4492-4492: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4552-4552: Use
key not in dictinstead ofkey not in dict.keys()Remove
.keys()(SIM118)
4672-4672: Use
action_config.get("website")instead ofaction_config.get("website", None)Replace
action_config.get("website", None)withaction_config.get("website")(SIM910)
4713-4713: Use
action_config.get("website")instead ofaction_config.get("website", None)Replace
action_config.get("website", None)withaction_config.get("website")(SIM910)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kairon/shared/account/processor.py(2 hunks)kairon/shared/data/processor.py(18 hunks)tests/unit_test/api/api_processor_test.py(1 hunks)tests/unit_test/data_processor/data_processor2_test.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/api/api_processor_test.py
128-128: Local variable account is assigned to but never used
Remove assignment to unused variable account
(F841)
129-129: Local variable user is assigned to but never used
Remove assignment to unused variable user
(F841)
130-130: Local variable is_new_account is assigned to but never used
Remove assignment to unused variable is_new_account
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
🔇 Additional comments (22)
kairon/shared/account/processor.py (2)
172-173: LGTM! Name validation logic is well placed.The validation ensures that bot names only contain letters, numbers and underscores, with a clear error message.
228-229: LGTM! Consistent validation across methods.The validation logic matches the add_bot method, maintaining consistency in name validation across the codebase.
tests/unit_test/data_processor/data_processor2_test.py (3)
217-240: LGTM! Comprehensive test coverage for lookup name validation.Test cases thoroughly validate different invalid characters in lookup names.
242-285: LGTM! Thorough test coverage for synonym name validation.Test cases comprehensively validate different invalid characters in synonym names.
357-380: LGTM! Comprehensive test coverage for regex name validation.Test cases thoroughly validate different invalid characters in regex names.
tests/unit_test/api/api_processor_test.py (1)
125-146: LGTM! Test cases cover invalid bot name validation.The test implementation effectively verifies that bot names cannot contain special characters or spaces during updates. The error message validation using regex pattern matching is a good practice.
🧰 Tools
🪛 Ruff (0.8.2)
128-128: Local variable
accountis assigned to but never usedRemove assignment to unused variable
account(F841)
129-129: Local variable
useris assigned to but never usedRemove assignment to unused variable
user(F841)
130-130: Local variable
is_new_accountis assigned to but never usedRemove assignment to unused variable
is_new_account(F841)
kairon/shared/data/processor.py (16)
4134-4136: LGTM: Name validation added for HTTP action configThe validation ensures action names only contain letters, numbers and underscores, with a clear error message.
4196-4198: LGTM: Name validation added for HTTP action config creationConsistent validation check and error message for new HTTP action names.
4298-4300: LGTM: Name validation added for pyscript action creationProper validation of pyscript action names before creation.
4334-4336: LGTM: Name validation added for pyscript action updateConsistent validation check when updating pyscript action names.
4388-4390: LGTM: Name validation added for database action updateProper validation of database action names during updates.
4428-4430: LGTM: Name validation added for database action creationConsistent validation check for new database action names.
4577-4579: LGTM: Name validation added for Google search action creationProper validation of Google search action names.
4614-4616: LGTM: Name validation added for Google search action updateConsistent validation check when updating Google search action names.
4772-4774: LGTM: Name validation added for slot creationProper validation of slot names before creation.
4733-4735: LGTM: Name validation added for synonym editingConsistent validation check when editing synonym names.
4807-4809: LGTM: Name validation added for callback action creationProper validation of callback action names.
4487-4489: LGTM: Name validation added for callback action updateConsistent validation check when updating callback action names.
4555-4557: LGTM: Name validation added for schedule action creationProper validation of schedule action names.
4592-4594: LGTM: Name validation added for schedule action updateConsistent validation check when updating schedule action names.
4751-4753: LGTM: Name validation added for prompt action creationProper validation of prompt action names.
4806-4808: LGTM: Name validation added for prompt action updateConsistent validation check when updating prompt action names.
…ases for the same.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
kairon/shared/utils.py (1)
686-695: Consider consolidating special character validation methods.The new
contains_special_charactersmethod is very similar to the existingspecial_matchmethod at line 676. The only difference is in the regex pattern. Consider refactoring to avoid code duplication.Here's a suggested refactor that makes the code more DRY:
- @staticmethod - def contains_special_characters(strg, search=re.compile(r"[^a-zA-Z0-9 _-]").search): - """ - Check if the string contains special characters other than allowed ones (space, _ and -). - - :param strg: text value - :param search: search pattern - :return: boolean - """ - return bool(search(strg)) @staticmethod - def special_match(strg, search=re.compile(r"[^a-zA-Z0-9_]").search): + def special_match(strg, allowed_chars="_", search=None): """ - check if string contains special character other than allowed ones + Check if string contains special characters other than allowed ones. :param strg: text value - :param search: search pattern + :param allowed_chars: string of allowed characters besides alphanumeric + :param search: optional pre-compiled regex search pattern :return: boolean """ + if search is None: + pattern = f"[^a-zA-Z0-9{re.escape(allowed_chars)}]" + search = re.compile(pattern).search return bool(search(strg)) + @staticmethod + def contains_special_characters(strg): + """ + Check if the string contains special characters other than allowed ones (space, _ and -). + + :param strg: text value + :return: boolean + """ + return Utility.special_match(strg, allowed_chars=" _-")This refactor:
- Makes
special_matchmore flexible by accepting allowed characters as a parameter- Reuses
special_matchincontains_special_characters- Maintains backward compatibility
- Reduces code duplication
tests/unit_test/utility_test.py (1)
3516-3576: Well-structured test cases for Messenger paragraph transformer!The test suite maintains consistency with other platform tests while handling Messenger-specific requirements.
Consider refactoring these platform-specific test suites to reduce code duplication by:
- Creating shared test fixtures
- Using parameterized tests for common scenarios
- Maintaining platform-specific tests only for unique behaviors
tests/unit_test/api/api_processor_test.py (2)
104-127: LGTM! Comprehensive test coverage for bot name validation.The test method effectively validates bot name restrictions by testing multiple invalid patterns with special characters (#, @, &).
Remove unused variable assignments.
The variables
account,user, andis_new_accountare assigned but never used in the test assertions.- account = pytest.account - user = "fshaikh@digite.com" - is_new_account = True with pytest.raises( AppException, match=re.escape("Invalid name! Use only letters, numbers, spaces, hyphens (-), and underscores (_).")): name = "test#21" - AccountProcessor.add_bot(name=name, account=account, user=user, is_new_account=is_new_account) + AccountProcessor.add_bot(name=name, account=pytest.account, user="fshaikh@digite.com", is_new_account=True)
128-152: LGTM! Comprehensive test coverage for bot name validation during updates.The test method effectively validates bot name restrictions by testing multiple invalid patterns with special characters (?, (), <>).
Remove unused variables and inline single-use variable.
The variables
account,user,is_new_accountare assigned but never used, andbotis used only once.- account = pytest.account - user = "fshaikh@digite.com" - is_new_account = True - bot = "test_bot" with pytest.raises( AppException, match=re.escape("Invalid name! Use only letters, numbers, spaces, hyphens (-), and underscores (_).")): name = "test?17" - AccountProcessor.update_bot(name=name, bot=bot) + AccountProcessor.update_bot(name=name, bot="test_bot")🧰 Tools
🪛 Ruff (0.8.2)
131-131: Local variable
accountis assigned to but never usedRemove assignment to unused variable
account(F841)
132-132: Local variable
useris assigned to but never usedRemove assignment to unused variable
user(F841)
133-133: Local variable
is_new_accountis assigned to but never usedRemove assignment to unused variable
is_new_account(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kairon/shared/account/processor.py(2 hunks)kairon/shared/utils.py(1 hunks)tests/unit_test/api/api_processor_test.py(1 hunks)tests/unit_test/utility_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kairon/shared/account/processor.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/api/api_processor_test.py
131-131: Local variable account is assigned to but never used
Remove assignment to unused variable account
(F841)
132-132: Local variable user is assigned to but never used
Remove assignment to unused variable user
(F841)
133-133: Local variable is_new_account is assigned to but never used
Remove assignment to unused variable is_new_account
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
tests/unit_test/utility_test.py (3)
2809-2823: Well-structured test cases for special character validation!The test cases are comprehensive and cover important validation scenarios:
- Basic special character validation
- Handling of spaces, hyphens, and underscores (which should not be considered special)
- Proper handling of ampersand as a special character
3380-3451: Comprehensive test coverage for WhatsApp paragraph transformer!The test suite demonstrates good testing practices:
- Covers basic functionality, formatting, and edge cases
- Proper mocking of dependencies
- Includes error handling scenarios
- Clear assertions and expected outputs
3453-3513: Well-structured test cases for Telegram paragraph transformer!The test suite effectively:
- Mirrors the WhatsApp test structure for consistency
- Adapts assertions for Telegram-specific HTML formatting
- Maintains good coverage of edge cases and error scenarios
…ases for the same.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/data_processor/data_processor_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
- GitHub Check: Codacy Static Code Analysis
| processor = MongoProcessor() | ||
| bot = 'test' | ||
| user = 'test' | ||
| action = {'name': ' ', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value, | ||
| action = {'name': '_', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value, | ||
| 'value': 'name'}]} | ||
| with pytest.raises(AppException, match=f'Slot setting action with name "{action["name"]}" not found'): | ||
| processor.edit_slot_set_action(action, bot, user) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test case to properly validate component names.
The test case has several issues that need to be addressed to align with the PR's objective of validating component names:
- The test name
test_edit_slot_set_action_name_emptyis misleading as it's testing with an underscore ('_'), not an empty name. - The expected error message suggests a "not found" scenario rather than a validation failure, which doesn't align with the PR's goal of name validation.
Consider applying these changes:
-def test_edit_slot_set_action_name_empty():
+def test_edit_slot_set_action_invalid_name():
processor = MongoProcessor()
bot = 'test'
user = 'test'
action = {'name': '_', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value,
'value': 'name'}]}
- with pytest.raises(AppException, match=f'Slot setting action with name "{action["name"]}" not found'):
+ with pytest.raises(AppException, match='Action name must contain letters, numbers and underscores only'):
processor.edit_slot_set_action(action, bot, user)Additionally, consider adding more test cases to validate other invalid name patterns (e.g., special characters, spaces) to ensure comprehensive validation coverage.
📝 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.
| processor = MongoProcessor() | |
| bot = 'test' | |
| user = 'test' | |
| action = {'name': ' ', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value, | |
| action = {'name': '_', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value, | |
| 'value': 'name'}]} | |
| with pytest.raises(AppException, match=f'Slot setting action with name "{action["name"]}" not found'): | |
| processor.edit_slot_set_action(action, bot, user) | |
| def test_edit_slot_set_action_invalid_name(): | |
| processor = MongoProcessor() | |
| bot = 'test' | |
| user = 'test' | |
| action = {'name': '_', 'set_slots': [{'name': 'name', 'type': SLOT_SET_TYPE.FROM_VALUE.value, | |
| 'value': 'name'}]} | |
| with pytest.raises(AppException, match='Action name must contain letters, numbers and underscores only'): | |
| processor.edit_slot_set_action(action, bot, user) |
Added validations for all components names and added and fixed test cases for the same.
Summary by CodeRabbit
Bug Fixes
Tests