MGMT-21204: add categories to feedback API #273
Conversation
|
""" WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant FeedbackRequestModel
User->>API: Submit feedback (may include categories)
API->>FeedbackRequestModel: Validate feedback (sentiment, user_feedback, categories)
FeedbackRequestModel-->>API: Validation result
API-->>User: Acknowledge feedback receipt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
zszabo-rh
force-pushed
the
feedback_category
branch
from
a5508e7 to
318010a
Compare
| return None # Convert empty list to None for consistency | ||
|
|
||
| unique_categories = list(set(value)) | ||
| return unique_categories |
There was a problem hiding this comment.
Should this validation take into account the "sentiment" field ? Because from L224 says:
"List of feedback categories that describe issues with the LLM response (for negative feedback)."
So I assume, it's only required when feedback is negative.
There was a problem hiding this comment.
Currently in assisted chatbot we will use this endpoint that way, as it's more aligned with other implementations, so that the user either gives thumbs up, or thumbs down + optional comment.
But maybe even so the API should not have such constraint. There might be positive categories coming later.
There was a problem hiding this comment.
Gotcha, thanks for the explanation
| user_question="Test question", | ||
| llm_response="Test response", | ||
| categories="invalid_type", # Should be list, not string | ||
| ) |
There was a problem hiding this comment.
We need a test with a positive review (sentiment) and an empty list of categories. Because from what I understood with this change, categories is only required when the feedback is negative.
There was a problem hiding this comment.
same comment..
so far the user needed to provide either sentiment or feedback, or both
now category is just a 3rd option, any combination would do, until at least one of those is there
There was a problem hiding this comment.
Gotcha, thanks for the explanation
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
It LGTM, other teams should also look into this PR to see if it fits the expectations for their use of the feedback API.
| user_question="Test question", | ||
| llm_response="Test response", | ||
| categories="invalid_type", # Should be list, not string | ||
| ) |
There was a problem hiding this comment.
Gotcha, thanks for the explanation
| return None # Convert empty list to None for consistency | ||
|
|
||
| unique_categories = list(set(value)) | ||
| return unique_categories |
There was a problem hiding this comment.
Gotcha, thanks for the explanation
|
/cc @tisnik |
zszabo-rh
force-pushed
the
feedback_category
branch
from
318010a to
4b256f3
Compare
![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
🧹 Nitpick comments (2)
src/models/requests.py (1)
261-277: Validate the deduplication logic preserves order.The validation logic correctly handles None values, empty lists, and type validation. However, the deduplication using
list(set(value))may not preserve the original order of categories.Consider preserving order while deduplicating:
- unique_categories = list(set(value)) + # Preserve order while removing duplicates + seen = set() + unique_categories = [] + for category in value: + if category not in seen: + seen.add(category) + unique_categories.append(category)docs/openapi.md (1)
460-476: Documentation accurately reflects the enum implementation.The FeedbackCategory table clearly explains each category with appropriate descriptions that match the inline comments in the code.
Consider the static analysis suggestion to replace "completely" with a stronger adverb for better clarity:
-| incorrect | The answer provided is completely wrong | +| incorrect | The answer provided is entirely wrong |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/openapi.json(3 hunks)docs/openapi.md(3 hunks)src/models/requests.py(6 hunks)tests/unit/app/endpoints/test_feedback.py(3 hunks)tests/unit/models/test_requests.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/app/endpoints/test_feedback.py (2)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)src/app/endpoints/feedback.py (1)
store_feedback(104-128)
tests/unit/models/test_requests.py (1)
src/models/requests.py (2)
FeedbackRequest(166-291)FeedbackCategory(150-163)
🪛 LanguageTool
docs/openapi.md
[style] ~470-~470: Consider using a different adverb to strengthen your wording.
Context: ...-| | incorrect | The answer provided is completely wrong | | not_relevant | This answer do...
(COMPLETELY_ENTIRELY)
🪛 markdownlint-cli2 (0.17.2)
docs/openapi.md
502-502: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (10)
src/models/requests.py (3)
150-164: Well-designed enum with clear categorization for negative feedback.The
FeedbackCategoryenum is well-structured with descriptive values and inline example comments. The categories cover the primary types of issues users might encounter with AI responses.
201-209: Categories field implementation looks solid.The field definition is well-documented with appropriate type hints, description, and examples. The multi-select capability aligns well with the PR objectives for structured feedback collection.
279-291: Model validation correctly handles the expanded feedback requirements.The updated validator properly ensures at least one form of feedback is provided among sentiment, user_feedback, or categories. This addresses the past review discussion about flexibility in feedback types.
docs/openapi.md (1)
489-522: Examples demonstrate proper usage patterns.The updated documentation includes comprehensive examples showing both basic feedback and feedback with categories, which helps users understand the multi-select capability.
tests/unit/models/test_requests.py (3)
8-8: Import addition supports new test functionality.The
FeedbackCategoryimport enables testing of the new enum functionality.
211-223: Test method name and logic correctly updated.The test name change from
test_check_sentiment_or_user_feedbacktotest_check_feedback_providedaccurately reflects the expanded validation logic that now includes categories.
237-347: Comprehensive test coverage for categories functionality.The test suite thoroughly covers all aspects of the categories field:
- Basic construction with multiple categories
- Single category usage
- Deduplication logic
- Empty list handling
- Categories-only feedback validation
- Mixed feedback types
- All enum values validation
- Invalid type handling
This provides excellent coverage of the new functionality and edge cases.
tests/unit/app/endpoints/test_feedback.py (2)
51-76: Parameterized tests effectively cover multiple feedback scenarios.The parameterization approach cleanly tests both scenarios (with and without categories) while maintaining the existing test structure. The test IDs provide clear identification of each scenario.
113-154: Store feedback tests cover diverse payload structures.The parameterized tests validate that the storage functionality works correctly with both traditional text feedback and the new categories-based feedback, ensuring backward compatibility.
docs/openapi.json (1)
938-958: Verifyx-enum-varnamessupport in downstream tooling
x-enum-varnamesis a Pydantic/FastAPI-specific extension rather than a standard OpenAPI 3.1 keyword.
If any of your client-side code generators, documentation builders, or validation gateways read the raw JSON, double-check they gracefully ignore or properly interpret this extension; otherwise they may flag the schema as invalid.
No action needed if your pipeline is already FastAPI-aware, but worth confirming before merging.
| "categories": { | ||
| "anyOf": [ | ||
| { | ||
| "items": { | ||
| "$ref": "#/components/schemas/FeedbackCategory" | ||
| }, | ||
| "type": "array" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "title": "Categories", | ||
| "description": "List of feedback categories that apply to the LLM response.", | ||
| "examples": [ | ||
| [ | ||
| "incorrect", | ||
| "incomplete" | ||
| ] | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema does not enforce non-empty & unique categories list
The server-side model requires the list to be non-empty & de-duplicated, but the OpenAPI schema leaves those constraints implicit.
Adding minItems and uniqueItems makes the contract self-describing and prevents clients from sending invalid payloads that will be rejected at runtime.
"items": {
"$ref": "#/components/schemas/FeedbackCategory"
},
"type": "array",
+ "minItems": 1,
+ "uniqueItems": true📝 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.
| "categories": { | |
| "anyOf": [ | |
| { | |
| "items": { | |
| "$ref": "#/components/schemas/FeedbackCategory" | |
| }, | |
| "type": "array" | |
| }, | |
| { | |
| "type": "null" | |
| } | |
| ], | |
| "title": "Categories", | |
| "description": "List of feedback categories that apply to the LLM response.", | |
| "examples": [ | |
| [ | |
| "incorrect", | |
| "incomplete" | |
| ] | |
| ] | |
| "categories": { | |
| "anyOf": [ | |
| { | |
| "items": { | |
| "$ref": "#/components/schemas/FeedbackCategory" | |
| }, | |
| "type": "array", | |
| "minItems": 1, | |
| "uniqueItems": true | |
| }, | |
| { | |
| "type": "null" | |
| } | |
| ], | |
| "title": "Categories", | |
| "description": "List of feedback categories that apply to the LLM response.", | |
| "examples": [ | |
| [ | |
| "incorrect", | |
| "incomplete" | |
| ] | |
| ] | |
| } |
🤖 Prompt for AI Agents
In docs/openapi.json around lines 1000 to 1019, the schema for the "categories"
array does not enforce that the list is non-empty and contains unique items. To
fix this, add "minItems": 1 and "uniqueItems": true to the array schema under
"categories" to explicitly require at least one item and ensure all items are
unique, aligning the schema with the server-side model constraints.
4 participants
Description
This PR introduces structured, multi-select “quick response” feedback categories for negative (thumbs-down) user feedback and removes obsolete / positive categories.
Key points
FeedbackCategoryenum (6 values) – negative-only categories (incorrect,not_relevant,incomplete,outdated_information,unsafe,other) with example comments for each.FeedbackRequestmodel updatedcategories: list[FeedbackCategory] | Nonesentiment,user_feedback, orcategoriesis provided./v1/feedbackOpenAPI schema +docs/openapi.(json|md)updated with new field, examples and category table.Type of change
Checklist before requesting a review
Testing
1. Unit tests
uv run pytest tests/unit/ -k "feedback" -vAll feedback-related tests pass.
2. Manual API verification
Expected response:
{"response":"feedback received"}(HTTP 200).Summary by CodeRabbit
Summary by CodeRabbit