Skip to content

Commit 552bb04

Browse files
max-svistunovclaude
andcommitted
Fix unit tests by mocking retrieve_conversation
Apply CodeRabbit suggestion to combine two DB queries into one by checking conversation.user_id directly instead of calling validate_conversation_ownership. Update unit tests to mock retrieve_conversation to avoid database initialization errors during testing. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 719977e commit 552bb04

File tree

2 files changed

+27
-9
lines changed

2 files changed

+27
-9
lines changed

src/app/endpoints/feedback.py

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,7 @@
2424
StatusResponse,
2525
UnauthorizedResponse,
2626
)
27-
from utils.endpoints import (
28-
check_configuration_loaded,
29-
retrieve_conversation,
30-
validate_conversation_ownership,
31-
)
27+
from utils.endpoints import check_configuration_loaded, retrieve_conversation
3228
from utils.suid import get_suid
3329

3430
logger = logging.getLogger(__name__)
@@ -125,7 +121,7 @@ async def feedback_endpoint_handler(
125121
user_id, _, _, _ = auth
126122
check_configuration_loaded(configuration)
127123

128-
# Validate conversation exists
124+
# Validate conversation exists and belongs to the user
129125
conversation_id = feedback_request.conversation_id
130126
conversation = retrieve_conversation(conversation_id)
131127
if conversation is None:
@@ -134,9 +130,7 @@ async def feedback_endpoint_handler(
134130
)
135131
raise HTTPException(**response.model_dump())
136132

137-
# Validate conversation belongs to the user
138-
owned_conversation = validate_conversation_ownership(user_id, conversation_id)
139-
if owned_conversation is None:
133+
if conversation.user_id != user_id:
140134
response = ForbiddenResponse.conversation(
141135
action="submit feedback for", resource_id=conversation_id, user_id=user_id
142136
)

tests/unit/app/endpoints/test_feedback.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,17 @@ async def test_feedback_endpoint_handler(
102102
mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None)
103103
mocker.patch("app.endpoints.feedback.store_feedback", return_value=None)
104104

105+
# Mock retrieve_conversation to return a conversation owned by test_user_id
106+
mock_conversation = mocker.Mock()
107+
mock_conversation.user_id = "test_user_id"
108+
mocker.patch(
109+
"app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation
110+
)
111+
105112
# Prepare the feedback request mock
106113
feedback_request = mocker.Mock()
107114
feedback_request.model_dump.return_value = feedback_request_data
115+
feedback_request.conversation_id = "12345678-abcd-0000-0123-456789abcdef"
108116

109117
# Authorization tuple required by URL endpoint handler
110118
auth: AuthTuple = ("test_user_id", "test_user", True, "test_token")
@@ -126,6 +134,14 @@ async def test_feedback_endpoint_handler_error(mocker: MockerFixture) -> None:
126134
mock_authorization_resolvers(mocker)
127135
mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None)
128136
mocker.patch("app.endpoints.feedback.check_configuration_loaded", return_value=None)
137+
138+
# Mock retrieve_conversation to return a conversation owned by test_user_id
139+
mock_conversation = mocker.Mock()
140+
mock_conversation.user_id = "test_user_id"
141+
mocker.patch(
142+
"app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation
143+
)
144+
129145
# Mock Path.mkdir to raise OSError so the try block in store_feedback catches it
130146
mocker.patch(
131147
"app.endpoints.feedback.Path.mkdir", side_effect=OSError("Permission denied")
@@ -300,6 +316,14 @@ async def test_feedback_endpoint_valid_requests(
300316
"""Test endpoint with valid feedback payloads."""
301317
mock_authorization_resolvers(mocker)
302318
mocker.patch("app.endpoints.feedback.store_feedback")
319+
320+
# Mock retrieve_conversation to return a conversation owned by mock_user_id
321+
mock_conversation = mocker.Mock()
322+
mock_conversation.user_id = "mock_user_id"
323+
mocker.patch(
324+
"app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation
325+
)
326+
303327
request = FeedbackRequest(**{**VALID_BASE, **payload})
304328
response = await feedback_endpoint_handler(
305329
feedback_request=request,

0 commit comments

Comments
 (0)