Skip to content

Commit 26505ca

Browse files
committed
fix unit test failures
1 parent d6344df commit 26505ca

File tree

8 files changed

+199
-12
lines changed

8 files changed

+199
-12
lines changed

src/app/endpoints/conversations.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,20 +190,27 @@ def get_conversations_list_endpoint_handler(
190190
]
191191

192192
logger.info(
193-
"Found %d conversations for user %s", len(conversations), user_id
193+
"Found %d conversations for anonymous user %s",
194+
len(conversations),
195+
anonymous_user_id,
194196
)
195197

196198
return ConversationsListResponse(conversations=conversations)
197199

198200
except Exception as e:
199201
logger.exception(
200-
"Error retrieving conversations for user %s: %s", user_id, e
202+
"Error retrieving conversations for anonymous user %s: %s",
203+
anonymous_user_id,
204+
e,
201205
)
202206
raise HTTPException(
203207
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
204208
detail={
205209
"response": "Unknown error",
206-
"cause": f"Unknown error while getting conversations for user {user_id}",
210+
"cause": (
211+
f"Unknown error while getting conversations for "
212+
f"anonymous user {anonymous_user_id}"
213+
),
207214
},
208215
) from e
209216

src/app/endpoints/feedback.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
)
2020
from models.requests import FeedbackRequest
2121
from utils.suid import get_suid
22+
from utils.user_anonymization import get_anonymous_user_id
2223

2324
logger = logging.getLogger(__name__)
2425
router = APIRouter(prefix="/feedback", tags=["feedback"])
@@ -131,7 +132,8 @@ def store_feedback(user_id: str, feedback: dict) -> None:
131132
user_id (str): Unique identifier of the user submitting feedback.
132133
feedback (dict): Feedback data to be stored, merged with user ID and timestamp.
133134
"""
134-
logger.debug("Storing feedback for user %s", user_id)
135+
anonymous_user_id = get_anonymous_user_id(user_id)
136+
logger.debug("Storing feedback for anonymous user %s", anonymous_user_id)
135137
# Creates storage path only if it doesn't exist. The `exist_ok=True` prevents
136138
# race conditions in case of multiple server instances trying to set up storage
137139
# at the same location.
@@ -141,7 +143,11 @@ def store_feedback(user_id: str, feedback: dict) -> None:
141143
storage_path.mkdir(parents=True, exist_ok=True)
142144

143145
current_time = str(datetime.now(UTC))
144-
data_to_store = {"user_id": user_id, "timestamp": current_time, **feedback}
146+
data_to_store = {
147+
"anonymous_user_id": anonymous_user_id,
148+
"timestamp": current_time,
149+
**feedback,
150+
}
145151

146152
# stores feedback in a file under unique uuid
147153
feedback_file_path = storage_path / f"{get_suid()}.json"

tests/unit/app/endpoints/test_conversations.py

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ def create_mock_conversation(
4646

4747
def mock_database_session(mocker, query_result=None):
4848
"""Helper function to mock get_session with proper context manager support."""
49+
# Mock database initialization
50+
mocker.patch("app.database.engine", mocker.Mock())
51+
mocker.patch("app.database.SessionLocal", mocker.Mock())
52+
4953
mock_session = mocker.Mock()
5054
if query_result is not None:
5155
mock_session.query.return_value.filter_by.return_value.all.return_value = (
@@ -530,6 +534,10 @@ def test_configuration_not_loaded(self, mocker):
530534
def test_successful_conversations_list_retrieval(self, mocker, setup_configuration):
531535
"""Test successful retrieval of conversations list."""
532536
mocker.patch("app.endpoints.conversations.configuration", setup_configuration)
537+
mocker.patch(
538+
"app.endpoints.conversations.get_anonymous_user_id",
539+
return_value="anon-test-user",
540+
)
533541

534542
# Mock database session and query results
535543
mock_conversations = [
@@ -570,6 +578,10 @@ def test_successful_conversations_list_retrieval(self, mocker, setup_configurati
570578
def test_empty_conversations_list(self, mocker, setup_configuration):
571579
"""Test when user has no conversations."""
572580
mocker.patch("app.endpoints.conversations.configuration", setup_configuration)
581+
mocker.patch(
582+
"app.endpoints.conversations.get_anonymous_user_id",
583+
return_value="anon-test-user",
584+
)
573585

574586
# Mock database session with no results
575587
mock_database_session(mocker, [])
@@ -583,6 +595,10 @@ def test_empty_conversations_list(self, mocker, setup_configuration):
583595
def test_database_exception(self, mocker, setup_configuration):
584596
"""Test when database query raises an exception."""
585597
mocker.patch("app.endpoints.conversations.configuration", setup_configuration)
598+
mocker.patch(
599+
"app.endpoints.conversations.get_anonymous_user_id",
600+
return_value="anon-test-user",
601+
)
586602

587603
# Mock database session to raise exception
588604
mock_session = mock_database_session(mocker)
@@ -594,6 +610,6 @@ def test_database_exception(self, mocker, setup_configuration):
594610
assert exc_info.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
595611
assert "Unknown error" in exc_info.value.detail["response"]
596612
assert (
597-
"Unknown error while getting conversations for user"
613+
"Unknown error while getting conversations for anonymous user"
598614
in exc_info.value.detail["cause"]
599615
)

tests/unit/app/endpoints/test_feedback.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ def test_store_feedback(mocker, feedback_request_data):
137137
mocker.patch("builtins.open", mocker.mock_open())
138138
mocker.patch("app.endpoints.feedback.Path", return_value=mocker.MagicMock())
139139
mocker.patch("app.endpoints.feedback.get_suid", return_value="fake-uuid")
140+
mocker.patch(
141+
"app.endpoints.feedback.get_anonymous_user_id", return_value="anon-test-user"
142+
)
140143

141144
# Patch json to inspect stored data
142145
mock_json = mocker.patch("app.endpoints.feedback.json")
@@ -146,7 +149,7 @@ def test_store_feedback(mocker, feedback_request_data):
146149
store_feedback(user_id, feedback_request_data)
147150

148151
expected_data = {
149-
"user_id": user_id,
152+
"anonymous_user_id": "anon-test-user",
150153
"timestamp": mocker.ANY,
151154
**feedback_request_data,
152155
}
@@ -182,6 +185,9 @@ def test_store_feedback_on_io_error(mocker, feedback_request_data):
182185
configuration.user_data_collection_configuration.feedback_storage = "fake-path"
183186
mocker.patch("app.endpoints.feedback.Path", return_value=mocker.MagicMock())
184187
mocker.patch("builtins.open", side_effect=PermissionError("EACCES"))
188+
mocker.patch(
189+
"app.endpoints.feedback.get_anonymous_user_id", return_value="anon-test-user"
190+
)
185191

186192
user_id = "test_user_id"
187193

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
"""Tests for feedback endpoint anonymization functionality."""
2+
3+
import os
4+
from unittest.mock import patch, MagicMock
5+
6+
import pytest
7+
8+
from app.endpoints.feedback import store_feedback
9+
10+
11+
# Set up test environment variable before importing the module
12+
@pytest.fixture(autouse=True)
13+
def setup_test_pepper():
14+
"""Set up test pepper environment variable for all tests."""
15+
test_pepper = "test-pepper-for-feedback-tests"
16+
with patch.dict(os.environ, {"USER_ANON_PEPPER": test_pepper}):
17+
yield
18+
19+
20+
class TestFeedbackAnonymization:
21+
"""Test feedback storage with user anonymization."""
22+
23+
@patch("app.endpoints.feedback.get_anonymous_user_id")
24+
@patch("app.endpoints.feedback.get_suid")
25+
@patch("app.endpoints.feedback.json")
26+
@patch("app.endpoints.feedback.Path")
27+
def test_store_feedback_anonymizes_user_id(
28+
self, mock_path, mock_json, mock_get_suid, mock_get_anonymous
29+
):
30+
"""Test that store_feedback uses anonymous user ID."""
31+
# Setup mocks
32+
mock_get_anonymous.return_value = "anon-feedback-123"
33+
mock_get_suid.return_value = "feedback-uuid"
34+
mock_path.return_value = MagicMock()
35+
36+
# Mock configuration
37+
with (
38+
patch("app.endpoints.feedback.configuration") as mock_config,
39+
patch("builtins.open"),
40+
):
41+
mock_config.user_data_collection_configuration.feedback_storage = (
42+
"/tmp/feedback"
43+
)
44+
45+
# Call store_feedback
46+
store_feedback(
47+
user_id="[email protected]",
48+
feedback={
49+
"feedback": "This is test feedback",
50+
"sentiment": 1,
51+
"categories": ["helpful"],
52+
},
53+
)
54+
55+
# Verify anonymous user ID was used
56+
mock_get_anonymous.assert_called_once_with("[email protected]")
57+
58+
# Verify stored data uses anonymous ID
59+
stored_data = mock_json.dump.call_args[0][0]
60+
assert stored_data["anonymous_user_id"] == "anon-feedback-123"
61+
assert "user_id" not in stored_data
62+
assert stored_data["feedback"] == "This is test feedback"
63+
assert stored_data["sentiment"] == 1
64+
assert stored_data["categories"] == ["helpful"]
65+
66+
@patch("app.endpoints.feedback.get_anonymous_user_id")
67+
def test_store_feedback_different_users_get_different_anonymous_ids(
68+
self, mock_get_anonymous
69+
):
70+
"""Test that different users get different anonymous IDs for feedback."""
71+
72+
def mock_anonymous_side_effect(user_id):
73+
if user_id == "[email protected]":
74+
return "anon-feedback-user1"
75+
if user_id == "[email protected]":
76+
return "anon-feedback-user2"
77+
return "anon-unknown"
78+
79+
mock_get_anonymous.side_effect = mock_anonymous_side_effect
80+
81+
with (
82+
patch("app.endpoints.feedback.json") as mock_json,
83+
patch("app.endpoints.feedback.Path") as mock_path,
84+
patch("builtins.open"),
85+
patch("app.endpoints.feedback.get_suid", return_value="uuid"),
86+
patch("app.endpoints.feedback.configuration") as mock_config,
87+
):
88+
89+
mock_config.user_data_collection_configuration.feedback_storage = (
90+
"/tmp/feedback"
91+
)
92+
mock_path.return_value = MagicMock()
93+
94+
# Store feedback for user 1
95+
store_feedback("[email protected]", {"feedback": "Test 1"})
96+
first_call_data = mock_json.dump.call_args[0][0]
97+
98+
# Reset mock for second call
99+
mock_json.reset_mock()
100+
101+
# Store feedback for user 2
102+
store_feedback("[email protected]", {"feedback": "Test 2"})
103+
second_call_data = mock_json.dump.call_args[0][0]
104+
105+
# Verify different anonymous IDs were used
106+
assert first_call_data["anonymous_user_id"] == "anon-feedback-user1"
107+
assert second_call_data["anonymous_user_id"] == "anon-feedback-user2"
108+
assert (
109+
first_call_data["anonymous_user_id"]
110+
!= second_call_data["anonymous_user_id"]
111+
)
112+
113+
@patch("app.endpoints.feedback.get_anonymous_user_id")
114+
@patch("app.endpoints.feedback.logger")
115+
def test_feedback_logging_uses_anonymous_id(self, mock_logger, mock_get_anonymous):
116+
"""Test that feedback logging uses anonymous user ID."""
117+
mock_get_anonymous.return_value = "anon-feedback-logging"
118+
119+
with (
120+
patch("app.endpoints.feedback.json"),
121+
patch("app.endpoints.feedback.Path") as mock_path,
122+
patch("builtins.open"),
123+
patch("app.endpoints.feedback.get_suid", return_value="uuid"),
124+
patch("app.endpoints.feedback.configuration") as mock_config,
125+
):
126+
127+
mock_config.user_data_collection_configuration.feedback_storage = (
128+
"/tmp/feedback"
129+
)
130+
mock_path.return_value = MagicMock()
131+
132+
# Store feedback
133+
store_feedback("[email protected]", {"feedback": "Test feedback"})
134+
135+
# Verify logging uses anonymous ID
136+
mock_logger.debug.assert_called_once_with(
137+
"Storing feedback for anonymous user %s", "anon-feedback-logging"
138+
)

tests/unit/app/endpoints/test_query.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,17 +169,17 @@ async def _test_query_endpoint_handler(mocker, store_transcript_to_file=False):
169169
# Assert the store_transcript function is called if transcripts are enabled
170170
if store_transcript_to_file:
171171
mock_transcript.assert_called_once_with(
172-
anonymous_user_id="mock_user_id",
172+
user_id="mock_user_id",
173173
conversation_id=conversation_id,
174174
model_id="fake_model_id",
175175
provider_id="fake_provider_id",
176176
query_is_valid=True,
177177
query=query,
178178
query_request=query_request,
179179
summary=summary,
180-
attachments=[],
181180
rag_chunks=[],
182181
truncated=False,
182+
attachments=[],
183183
)
184184
else:
185185
mock_transcript.assert_not_called()

tests/unit/utils/test_transcripts.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,18 @@ def test_construct_transcripts_path(mocker):
5050
def test_store_transcript(mocker):
5151
"""Test the store_transcript function."""
5252

53+
# Mock database initialization to prevent the error
54+
mocker.patch("app.database.engine", mocker.Mock())
55+
mocker.patch("app.database.SessionLocal", mocker.Mock())
56+
5357
mocker.patch("builtins.open", mocker.mock_open())
5458
mocker.patch(
5559
"utils.transcripts.construct_transcripts_path",
5660
return_value=mocker.MagicMock(),
5761
)
62+
mocker.patch(
63+
"utils.transcripts.get_anonymous_user_id", return_value="anon-user-123"
64+
)
5865

5966
# Mock the JSON to assert the data is stored correctly
6067
mock_json = mocker.patch("utils.transcripts.json")
@@ -104,7 +111,7 @@ def test_store_transcript(mocker):
104111
"model": "fake-model",
105112
"query_provider": query_request.provider,
106113
"query_model": query_request.model,
107-
"user_id": user_id,
114+
"anonymous_user_id": "anon-user-123",
108115
"conversation_id": conversation_id,
109116
"timestamp": mocker.ANY,
110117
},

tests/unit/utils/test_transcripts_anonymization.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,14 @@ def test_store_transcript_with_attachments(
213213
assert data["attachments"][0]["attachment_type"] == "text"
214214
assert data["attachments"][0]["content"] == "Test attachment content"
215215

216-
def test_path_sanitization_with_anonymous_ids(self):
216+
@patch("utils.transcripts.configuration")
217+
def test_path_sanitization_with_anonymous_ids(self, mock_config):
217218
"""Test that path sanitization works correctly with anonymous UUIDs."""
219+
# Setup mock configuration
220+
mock_config.user_data_collection_configuration.transcripts_storage = (
221+
"/tmp/transcripts"
222+
)
223+
218224
# Test with various UUID formats and potential path injection
219225
test_cases = [
220226
("anon-uuid-123", "conv-456"),
@@ -229,7 +235,8 @@ def test_path_sanitization_with_anonymous_ids(self):
229235

230236
# Should not contain path traversal sequences
231237
assert "../" not in result_str
232-
assert not result_str.startswith("/")
238+
# Paths should be absolute (start with /) since we use /tmp/transcripts as base
239+
assert result_str.startswith("/tmp/transcripts/")
233240

234241
@patch("utils.transcripts.get_anonymous_user_id")
235242
def test_logging_shows_anonymization(self, mock_get_anonymous, caplog):

0 commit comments

Comments
 (0)