Delete slot and slot mapping #1605
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the 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: 1
🧹 Outside diff range and nitpick comments (8)
tests/integration_test/services_test.py (3)
19589-19612: Enhance test robustness and clarityWhile the test covers the basic flow, consider these improvements:
- Rename test to better reflect its purpose (current name suggests a duplicate)
- Add cleanup in case of failure between operations
- Add validation of actual database state
- Document the significance of "na" and the mapping configuration
- Add negative test cases (e.g., invalid mapping types)
Consider refactoring to:
-def test_add_slot_mapping_2(): +def test_add_text_slot_with_from_text_mapping(): + """Test creation of a text slot and adding a from_text mapping to it. + The slot 'na' represents [explain significance]. + """ try: response = client.post( f"/api/bot/{pytest.bot}/slots", json={"name": "na", "type": "text"}, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() assert actual["message"] == "Slot added successfully!" assert actual["success"] assert actual["error_code"] == 0 + + # Verify slot exists in database + slot = mongo_processor.get_slot("na") + assert slot is not None response = client.post( f"/api/bot/{pytest.bot}/slots/mapping", json={ "slot": "na", "mapping": {"type": "from_text", "value": "user", "entity": "na"}, }, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() assert actual["message"] == "Slot mapping added" assert actual["success"] assert actual["error_code"] == 0 + + # Verify mapping exists in database + mapping = mongo_processor.get_slot_mapping("na") + assert mapping is not None + finally: + # Cleanup in case of failure + mongo_processor.delete_slot("na")
19614-19623: Add database state validation and negative test casesThe test verifies the API response but doesn't confirm the actual database state.
Consider enhancing the test:
def test_delete_slot_mapping_exists(): + """Test deletion of a slot mapping while preserving the slot itself.""" response = client.delete( f"/api/bot/{pytest.bot}/slots/mapping/na", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() assert actual["success"] assert actual["error_code"] == 0 assert actual["message"] == "Slot mapping deleted" + + # Verify mapping is deleted from database + mapping = mongo_processor.get_slot_mapping("na") + assert mapping is None + + # Verify slot still exists + slot = mongo_processor.get_slot("na") + assert slot is not NoneAlso consider adding a negative test case:
def test_delete_nonexistent_slot_mapping(): """Test deletion of a mapping that doesn't exist.""" response = client.delete( f"/api/bot/{pytest.bot}/slots/mapping/nonexistent", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) actual = response.json() assert not actual["success"] assert actual["error_code"] != 0
19589-19635: Address test dependencies and structureThe current tests form an implicit sequence (add slot -> add mapping -> delete mapping -> delete slot) without proper dependency management. This could lead to flaky tests and maintenance issues.
Consider these structural improvements:
- Use pytest's dependency management:
@pytest.mark.dependency() @pytest.mark.dependency(name="slot_created") def test_add_text_slot_with_from_text_mapping(): # ... existing code ... @pytest.mark.dependency(depends=["slot_created"]) @pytest.mark.dependency(name="mapping_deleted") def test_delete_slot_mapping_exists(): # ... existing code ... @pytest.mark.dependency(depends=["mapping_deleted"]) def test_delete_slots_exists(): # ... existing code ...
- Consider using fixtures for setup/teardown:
@pytest.fixture def text_slot(): """Fixture to create and clean up a text slot.""" response = client.post( f"/api/bot/{pytest.bot}/slots", json={"name": "test_slot", "type": "text"}, headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) yield response # Cleanup client.delete( f"/api/bot/{pytest.bot}/slots/test_slot", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, )
- Group related tests in a class:
@pytest.mark.slots class TestSlotOperations: """Tests for slot CRUD operations.""" def test_add_text_slot_with_from_text_mapping(self): # ... existing code ... def test_delete_slot_mapping_exists(self): # ... existing code ... def test_delete_slots_exists(self): # ... existing code ...kairon/shared/data/processor.py (3)
Line range hint
4791-4924: Comprehensive validation logic for slot deletion, but consider a few improvementsThe validation logic for slot deletion is thorough and checks for slot usage across multiple entities. However, there are a few suggestions to enhance the implementation:
- Consider extracting the validation logic into a separate method for better readability and reusability:
def validate_slot_dependencies(self, slot_name: Text, bot: Text): """ Validates if a slot can be deleted by checking its dependencies. :param slot_name: Name of the slot to validate :param bot: Bot ID :raises: AppException if slot has dependencies """ forms_with_slot = Forms.objects(bot=bot, status=True, required_slots__in=[slot_name]) if len(forms_with_slot) > 0: raise AppException(f'Slot is attached to form: {[form["name"] for form in forms_with_slot]}') # Add other validation checks...
- Consider adding batch validation capability to check multiple slots at once:
def validate_slots_dependencies(self, slot_names: List[Text], bot: Text): """ Validates if multiple slots can be deleted by checking their dependencies. :param slot_names: List of slot names to validate :param bot: Bot ID :returns: Dict mapping slot names to their dependencies """ dependencies = {} for slot_name in slot_names: try: self.validate_slot_dependencies(slot_name, bot) except AppException as e: dependencies[slot_name] = str(e) return dependenciesConsider implementing a dependency graph to track relationships between slots and other components. This would make it easier to:
- Visualize dependencies
- Identify orphaned slots
- Perform impact analysis before deletions
Line range hint
4791-4924: Consider enhancing error handling with specific exception typesThe current implementation uses generic AppException for all error cases. Consider creating specific exception types for different validation failures to enable more granular error handling by clients.
class SlotInUseError(AppException): """Raised when attempting to delete a slot that is in use""" pass class SystemSlotDeletionError(AppException): """Raised when attempting to delete a system slot""" pass def delete_slot(self, slot_name: Text, bot: Text, user: Text): try: if slot_name.lower() in {s.value for s in KaironSystemSlots}: raise SystemSlotDeletionError("Default kAIron slot deletion not allowed") if self.get_row_count(SlotMapping, bot, slot=slot_name, status=True) > 0: raise SlotInUseError("Cannot delete slot without removing its mappings!") # Rest of the validation logic... except DoesNotExist: raise AppException("Slot does not exist.")This would allow clients to handle different error cases differently:
try: processor.delete_slot(slot_name, bot, user) except SystemSlotDeletionError: # Handle system slot deletion attempt except SlotInUseError: # Handle slot in use case except AppException: # Handle other cases
Line range hint
4791-4924: Enhance method documentation with examples and error casesWhile the code is well-structured, the documentation could be enhanced to better describe the validation process and possible error scenarios.
Consider updating the docstring to:
def delete_slot(self, slot_name: Text, bot: Text, user: Text): """ Deletes a slot after validating that it's not in use. Args: slot_name: Name of the slot to delete bot: Bot ID user: User ID Raises: AppException: In the following cases: - Attempting to delete a system slot - Slot has active mappings - Slot is used in forms - Slot is used in actions - Slot is used in stories - Slot is used in training examples - Slot is used in multi-flow stories - Slot does not exist Example: try: processor.delete_slot("user_name", "bot123", "user456") except AppException as e: print(f"Could not delete slot: {str(e)}") """tests/unit_test/data_processor/data_processor_test.py (2)
9436-9449: Enhance test coverage with edge casesWhile the test verifies basic slot creation, consider adding the following test cases for better coverage:
- Values at boundaries (min_value=0.1, max_value=0.5)
- Values outside boundaries (<0.1, >0.5)
- Invalid case where min_value > max_value
def test_add_number_slot_with_invalid_min_max(self): processor = MongoProcessor() with pytest.raises(AppException, match="min_value cannot be greater than max_value"): processor.add_slot( {"name": "na", "type": "float", "initial_value": 0.2, "max_value": 0.1, "min_value": 0.5, "influence_conversation": True}, 'test', 'user' )
9461-9476: Consider test naming and error assertion improvements
The test name suffix
_1suggests there might be more test cases. Consider using more descriptive names liketest_delete_slot_fails_with_active_mappingandtest_delete_slot_succeeds_after_mapping_removal.The error message assertion could be more specific to ensure the exact error message format doesn't change unexpectedly.
- with pytest.raises(AppException, match="Cannot delete slot without removing its mappings!"): + expected_error = f"Cannot delete slot '{slot_name}' without removing its mappings!" + with pytest.raises(AppException, match=re.escape(expected_error)):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
kairon/shared/data/processor.py(2 hunks)tests/integration_test/services_test.py(1 hunks)tests/unit_test/data_processor/data_processor_test.py(1 hunks)
🔥 Files not summarized due to errors (1)
- tests/unit_test/data_processor/data_processor_test.py: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (3)
kairon/shared/data/processor.py (1)
Line range hint 4791-4924: Optimize database queries for slot dependency validation
The current implementation performs multiple separate queries to check slot dependencies. This could be optimized to reduce database round trips.
Consider using MongoDB's aggregation pipeline to check all dependencies in a single query:
def check_slot_dependencies(self, slot_name: Text, bot: Text):
"""
Checks slot dependencies using aggregation pipeline.
Returns a dict with counts of dependencies in each collection.
"""
pipeline = [
{
"$facet": {
"forms": [
{"$match": {"bot": bot, "status": True, "required_slots": slot_name}},
{"$count": "count"}
],
"google_actions": [
{"$match": {"bot": bot, "status": True, "set_slot": slot_name}},
{"$count": "count"}
],
"web_search_actions": [
{"$match": {"bot": bot, "status": True, "set_slot": slot_name}},
{"$count": "count"}
],
"stories": [
{
"$match": {
"bot": bot,
"status": True,
"events.name": slot_name,
"events.type": "slot"
}
},
{"$count": "count"}
],
"training_examples": [
{
"$match": {
"bot": bot,
"status": True,
"entities.entity": slot_name
}
},
{"$count": "count"}
]
}
}
]
results = Forms.objects().aggregate(pipeline)
return resultsThis would:
- Reduce the number of database queries
- Improve performance for bots with many components
- Make it easier to get a complete picture of slot usage
tests/unit_test/data_processor/data_processor_test.py (2)
9450-9460: LGTM! Good coverage of slot deletion
The test effectively verifies that the slot is properly removed from both Slots and Entities collections.
9450-9476: Consider adding these test scenarios
To ensure robust slot deletion functionality, consider adding tests for:
- Deleting a non-existent slot
- Deleting a slot used in training data or stories
- Concurrent deletion attempts
- Deletion with invalid bot/user credentials
| def test_delete_slots_exists(): | ||
| response = client.delete( | ||
| f"/api/bot/{pytest.bot}/slots/na", | ||
| headers={"Authorization": pytest.token_type + " " + pytest.access_token}, | ||
| ) | ||
|
|
||
| actual = response.json() | ||
| assert actual["message"] == "Slot deleted!" | ||
| assert actual["success"] | ||
| assert actual["error_code"] == 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and documentation
The test verifies the API response but lacks database validation and context.
Consider these improvements:
+@pytest.mark.dependency(depends=["test_delete_slot_mapping_exists"])
def test_delete_slots_exists():
+ """Test deletion of a slot after its mapping has been removed.
+ This test depends on test_delete_slot_mapping_exists having run first
+ to ensure the slot's mapping is already deleted.
+ """
response = client.delete(
f"/api/bot/{pytest.bot}/slots/na",
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
actual = response.json()
assert actual["message"] == "Slot deleted!"
assert actual["success"]
assert actual["error_code"] == 0
+
+ # Verify slot is deleted from database
+ slot = mongo_processor.get_slot("na")
+ assert slot is None
+
+ # Verify no orphaned references exist
+ forms = mongo_processor.get_forms()
+ for form in forms:
+ assert "na" not in form["required_slots"]Also add a test for the error case:
def test_delete_slot_with_existing_mapping():
"""Test that a slot cannot be deleted while it has an existing mapping."""
# First create a slot with mapping
client.post(
f"/api/bot/{pytest.bot}/slots",
json={"name": "test_slot", "type": "text"},
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
client.post(
f"/api/bot/{pytest.bot}/slots/mapping",
json={
"slot": "test_slot",
"mapping": {"type": "from_text", "value": "user", "entity": "test_slot"},
},
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
# Attempt to delete slot with existing mapping
response = client.delete(
f"/api/bot/{pytest.bot}/slots/test_slot",
headers={"Authorization": pytest.token_type + " " + pytest.access_token},
)
actual = response.json()
assert not actual["success"]
assert "Cannot delete slot with existing mapping" in actual["message"]
Changed logic related to Delete Slot and Slot Mapping and added related test cases.
Summary by CodeRabbit
New Features
Bug Fixes
Tests