diff --git a/src/forum/api/users.py b/src/forum/api/users.py index 8355f8d3..82c5d716 100644 --- a/src/forum/api/users.py +++ b/src/forum/api/users.py @@ -134,15 +134,8 @@ def retire_user( backend = get_backend(course_id)() user = backend.get_user(user_id) if not user: - raise ForumV2RequestError(f"user not found with id: {user_id}") - backend.update_user( - user_id, - data={ - "email": "", - "username": retired_username, - "read_states": [], - }, - ) + return {"message": f"User not found with id: {user_id}"} + backend.update_user(user_id, {"read_states": []}) backend.unsubscribe_all(user_id) backend.retire_all_content(user_id, retired_username) diff --git a/src/forum/backends/mysql/api.py b/src/forum/backends/mysql/api.py index bd4748f1..f47692d1 100644 --- a/src/forum/backends/mysql/api.py +++ b/src/forum/backends/mysql/api.py @@ -2017,8 +2017,6 @@ def update_user(user_id: str, data: dict[str, Any]) -> int: if "username" in data: user.username = data["username"] - if "email" in data: - user.email = data["email"] if "default_sort_key" in data: forum_user.default_sort_key = data["default_sort_key"] if "read_states" in data and data["read_states"] == []: diff --git a/tests/e2e/test_users.py b/tests/e2e/test_users.py index de1119e2..e64ec610 100644 --- a/tests/e2e/test_users.py +++ b/tests/e2e/test_users.py @@ -685,6 +685,8 @@ def test_retire_user_inactive(api_client: APIClient, patched_get_backend: Any) - backend = patched_get_backend() user_id = backend.find_or_create_user(user_id="1", username="user1") user = backend.get_user(user_id) or {} + original_username = user["username"] + original_email = user["email"] # Verify user is not subscribed to any threads response = api_client.get_json( @@ -709,9 +711,10 @@ def test_retire_user_inactive(api_client: APIClient, patched_get_backend: Any) - ) assert response.status_code == 200 + # Retiring user in the forum backend should not touch LMS User model. user = backend.get_user(user_id) or {} - assert user["username"] == retired_username - assert user["email"] == "" + assert user["username"] == original_username + assert user["email"] == original_email content = backend.get_user_contents_by_username(retired_username) assert len(content) == 0 diff --git a/tests/test_views/test_users.py b/tests/test_views/test_users.py index 9cdfaf62..4c96b3bb 100644 --- a/tests/test_views/test_users.py +++ b/tests/test_views/test_users.py @@ -1,9 +1,12 @@ """Tests for Users apis.""" from typing import Any +from unittest.mock import patch + import pytest from forum.constants import RETIRED_BODY, RETIRED_TITLE +from forum.utils import ForumV2RequestError from test_utils.client import APIClient pytestmark = pytest.mark.django_db @@ -377,23 +380,41 @@ def test_attempts_to_replace_username_without_sending_new_username( assert response.status_code == 500 +@pytest.mark.parametrize("data", [{}, {"unrelated_param": "value"}]) def test_attempts_to_retire_user_without_sending_retired_username( - api_client: APIClient, patched_get_backend: Any + api_client: APIClient, patched_get_backend: Any, data: dict[str, str] ) -> None: """Test retire user api without sending retired username.""" backend = patched_get_backend user_id = backend.generate_id() response = api_client.post_json( f"/api/v2/users/{user_id}/retire", - data={}, + data=data, ) assert response.status_code == 500 +def test_attempts_to_retire_user_with_backend_error(api_client: APIClient, patched_get_backend: Any) -> None: + """Test that a forum error raised while retiring a user is returned as a 400.""" + backend = patched_get_backend + user_id = backend.generate_id() + with patch("forum.views.users.retire_user", side_effect=ForumV2RequestError("some error")): + response = api_client.post_json( + f"/api/v2/users/{user_id}/retire", + data={"retired_username": "retired_user_test"}, + ) + assert response.status_code == 400 + assert response.json() == {"error": "some error"} + + def test_attempts_to_retire_non_existent_user( api_client: APIClient, patched_get_backend: Any ) -> None: - """Test retire non-existent user.""" + """ + Test retire non-existent user. + + Retiring a non-existent user should return 200, since the user can be considered already retired. + """ backend = patched_get_backend user_id = backend.generate_id() retired_username = "retired_user_test" @@ -401,7 +422,7 @@ def test_attempts_to_retire_non_existent_user( f"/api/v2/users/{user_id}/retire", data={"retired_username": retired_username}, ) - assert response.status_code == 400 + assert response.status_code == 200 def test_retire_user(api_client: APIClient, patched_get_backend: Any) -> None: @@ -416,6 +437,7 @@ def test_retire_user(api_client: APIClient, patched_get_backend: Any) -> None: setup_10_threads(user_id, username, backend) retired_username = "retired_username_ABCD1234" user = backend.get_user(user_id) + email = user["email"] assert user assert user["username"] == username @@ -426,8 +448,11 @@ def test_retire_user(api_client: APIClient, patched_get_backend: Any) -> None: assert response.status_code == 200 user = backend.get_user(user_id) assert user - assert user["username"] == retired_username - assert user["email"] == "" + + # Retiring user in the forum backend should not touch LMS User model. + assert user["username"] == username + assert user["email"] == email + contents = list(backend.get_contents(author_id=user_id)) assert len(contents) > 0 for content in contents: @@ -454,6 +479,7 @@ def test_retire_user_with_subscribed_threads( setup_10_threads(user_id, username, backend) retired_username = "retired_username_ABCD1234" user = backend.get_user(user_id) + email = user["email"] assert user assert user["username"] == username thread_id = backend.create_thread( @@ -483,8 +509,11 @@ def test_retire_user_with_subscribed_threads( user = backend.get_user(user_id) assert user - assert user["username"] == retired_username - assert user["email"] == "" + + # Retiring user in the forum backend should not touch LMS User model. + assert user["username"] == username + assert user["email"] == email + # User should be subscribed to no threads. response = api_client.get( f"/api/v2/users/{user_id}/subscribed_threads?course_id=course1",