Merge pull request lightspeed-core#711 from asimurka/fix_nonempty_feedback_must_be_provided

LCORE-755: Fixed feedback endpoint responds correctly without sentiment and empty user_feedback

Author: tisnik

src/models/requests.py

@@ -377,7 +377,7 @@ def check_feedback_provided(self) -> Self:
         """Ensure that at least one form of feedback is provided."""
         if (
             self.sentiment is None
-            and self.user_feedback is None
+            and (self.user_feedback is None or self.user_feedback == "")
             and self.categories is None
         ):
             raise ValueError(

tests/e2e/features/feedback.feature

@@ -306,4 +306,31 @@ Feature: feedback endpoint API tests
                         "cause": "[Errno 13] Permission denied: '/invalid'"
                     }
         }
-        """
+        """
+
+  Scenario: Check if feedback endpoint fails when only empty string user_feedback is provided
+    Given The system is in default state
+    And A new conversation is initialized
+    And The feedback is enabled
+     When I submit the following feedback for the conversation created before
+        """
+        {
+            "user_question": "Sample Question",
+            "llm_response": "Sample Response",
+            "user_feedback": ""
+        }
+        """
+     Then The status code of the response is 422
+     And the body of the response has the following structure
+        """
+        {
+            "detail": [{
+                        "type": "value_error", 
+                        "loc": ["body"], 
+                        "msg": "Value error, At least one form of feedback must be provided: 'sentiment', 'user_feedback', or 'categories'",
+                        "input": {
+                            "user_feedback": ""
+                        }
+                    }]           
+        }
+        """

tests/unit/app/endpoints/test_feedback.py

@@ -12,10 +12,18 @@
     store_feedback,
     update_feedback_status,
 )
-from models.requests import FeedbackStatusUpdateRequest
+from models.requests import FeedbackStatusUpdateRequest, FeedbackRequest
 from tests.unit.utils.auth_helpers import mock_authorization_resolvers
 
 
+MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")
+VALID_BASE = {
+    "conversation_id": "12345678-abcd-0000-0123-456789abcdef",
+    "user_question": "What is Kubernetes?",
+    "llm_response": "Kubernetes is an open-source container orchestration system.",
+}
+
+
 def test_is_feedback_enabled():
     """Test that is_feedback_enabled returns True when feedback is not disabled."""
     configuration.user_data_collection_configuration.feedback_enabled = True
@@ -230,3 +238,26 @@ async def test_update_feedback_status_no_change(mocker: MockerFixture):
         "updated_by": "test_user_id",
         "timestamp": mocker.ANY,
     }
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        {"sentiment": -1},
+        {"user_feedback": "Good answer"},
+        {"categories": ["incorrect"]},
+    ],
+    ids=["test_sentiment_only", "test_user_feedback_only", "test_categories_only"],
+)
+@pytest.mark.asyncio
+async def test_feedback_endpoint_valid_requests(mocker: MockerFixture, payload):
+    """Test endpoint with valid feedback payloads."""
+    mock_authorization_resolvers(mocker)
+    mocker.patch("app.endpoints.feedback.store_feedback")
+    request = FeedbackRequest(**{**VALID_BASE, **payload})
+    response = await feedback_endpoint_handler(
+        feedback_request=request,
+        auth=MOCK_AUTH,
+        _ensure_feedback_enabled=None,
+    )
+    assert response.response == "feedback received"

tests/unit/models/requests/test_feedback_request.py

@@ -182,3 +182,17 @@ def test_categories_invalid_type(self) -> None:
                 llm_response="Test response",
                 categories="invalid_type",  # Should be list, not string
             )
+
+    def test_empty_user_feedback_not_sufficient(self) -> None:
+        """Test that an empty string for user_feedback is treated as no feedback."""
+        with pytest.raises(
+            ValueError, match="At least one form of feedback must be provided"
+        ):
+            FeedbackRequest(
+                conversation_id="123e4567-e89b-12d3-a456-426614174000",
+                user_question="What is feedback?",
+                llm_response="Some LLM response.",
+                user_feedback="",  # Empty string should trigger validation error
+                sentiment=None,
+                categories=None,
+            )