Hide Default Slots from UI #1612
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Poem
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
🧹 Outside diff range and nitpick comments (1)
tests/integration_test/services_test.py (1)
9702-9712: Test modifications look good with a minor readability suggestion.The test changes effectively validate both the count and content of returned slots, ensuring system slots are properly excluded.
Consider extracting the magic number 21 to a constant for better maintainability:
+ TOTAL_SLOTS_COUNT = 21 # Total number of slots before exclusion - assert len(actual["data"]) == 21 - len(excluded_slots) + assert len(actual["data"]) == TOTAL_SLOTS_COUNT - len(excluded_slots)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
kairon/shared/data/processor.py(1 hunks)tests/integration_test/services_test.py(2 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 (2)
tests/integration_test/services_test.py (2)
50-50: LGTM!
The addition of KaironSystemSlots import is appropriate and well-placed with other constant imports.
9702-9712: Verify similar test cases for consistency.
While these changes look good, we should ensure other test cases that deal with slots are updated consistently.
Let's check for other test cases that might need similar updates:
✅ Verification successful
Let me gather more specific information about the test cases that might need updates.
Based on the analysis of the test files, I can now provide a final response:
No additional test updates required for system slot exclusion
The system slot exclusion logic is correctly implemented in the specific test case, and after reviewing related test cases, no other tests require similar updates because:
- The only slot count assertion in integration tests that needs to handle system slot exclusion is already updated in the current changes
- Other slot-related tests in the codebase are either:
- Unit tests that work with specific, controlled test data
- Tests that verify different aspects of slots (influence_conversation, mapping, etc.)
- Tests that don't deal with system slots enumeration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other test cases that might need similar updates
# Look for test cases that deal with slots and might need similar exclusion logic
# Search for test methods that deal with slots
rg -l "test.*slot" "tests/"
# Search for assertions on slot counts
rg "assert.*len.*slot" "tests/"
# Search for specific slot-related API calls
rg "get_existing_slots|slot.*response.json" "tests/"
Length of output: 4231
Script:
#!/bin/bash
# Get more context around slot-related test assertions
rg -B 5 -A 5 "assert.*len.*slot" tests/integration_test/services_test.py tests/unit_test/data_processor/data_processor_test.py
# Look for system slot related patterns
rg -B 3 -A 3 "system.*slot|slot.*system" tests/
Length of output: 17820
| ] | ||
| assert len(slots) == 24 | ||
| assert len(slots) == 10 |
There was a problem hiding this comment.
💡 Codebase verification
The test assertion needs to be updated to reflect system slots exclusion
Based on the implementation in get_existing_slots, the method now explicitly excludes system slots defined in KaironSystemSlots enum (which includes 'kairon_action_response', 'bot', 'image', 'audio', 'video'). However, the test still expects 10 slots in total, which doesn't align with this filtering behavior. The test should be updated to expect fewer slots, excluding these system slots.
tests/unit_test/data_processor/data_processor_test.py: Update the assertionassert len(slots) == 10to reflect the correct number of slots after system slots exclusion.
🔗 Analysis chain
Verify if this assertion aligns with hiding default slots
The assertion len(slots) == 10 seems to include all slots, which might be inconsistent with the PR objective of hiding default slots from the UI. Please verify if this test case should be updated to reflect the new behavior where default slots are excluded.
Let's verify the implementation of slot filtering:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of get_existing_slots to understand the expected behavior
ast-grep --pattern 'def get_existing_slots'
# Search for any constants or configurations defining default/system slots
rg -l "default.*slots|system.*slots" --type py
Length of output: 315
Script:
#!/bin/bash
# Let's examine the implementation of get_existing_slots in the processor
rg -A 10 "def get_existing_slots" kairon/shared/data/processor.py
# Look for the test context around the assertion
rg -B 5 -A 5 "assert len\(slots\) == 10" tests/unit_test/data_processor/data_processor_test.py
# Search for any system/default slots definitions
rg "DEFAULT_SLOTS|SYSTEM_SLOTS" --type py
Length of output: 1141
Script:
#!/bin/bash
# Let's look for KaironSystemSlots definition
rg -A 5 "class KaironSystemSlots" --type py
# Let's see the full test context to understand what slots are being tested
ast-grep --pattern 'def test_get_existing_slots'
# Look for any slot-related enums or constants
rg "class.*Slots.*\(Enum\)" --type py
Length of output: 464
| excluded_slots = list(KaironSystemSlots) | ||
| query = Q(bot=bot, status=True) & Q(name__nin=excluded_slots) | ||
| for slot in Slots.objects(query): |
There was a problem hiding this comment.
Critical Issue: Exclude system slot names correctly by extracting values
Currently, excluded_slots = list(KaironSystemSlots) will create a list of Enum members, not their values. This means the query name__nin=excluded_slots may not filter out the system slots as intended. To properly exclude the system slots by their names, you should extract the .value from each Enum member.
Apply this diff to fix the issue:
- excluded_slots = list(KaironSystemSlots)
+ excluded_slots = [slot.value for slot in KaironSystemSlots]📝 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.
| excluded_slots = list(KaironSystemSlots) | |
| query = Q(bot=bot, status=True) & Q(name__nin=excluded_slots) | |
| for slot in Slots.objects(query): | |
| excluded_slots = [slot.value for slot in KaironSystemSlots] | |
| query = Q(bot=bot, status=True) & Q(name__nin=excluded_slots) | |
| for slot in Slots.objects(query): |
Summary by CodeRabbit
New Features
Bug Fixes
Tests