From 9b483b9096fa7e4c4a30e0cb6c8821e41169847e Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Mon, 20 Oct 2025 23:45:04 +0200 Subject: [PATCH 1/2] fix: do not update LMS user during retirement 1. Modifying users is already handled by the LMS retirement pipeline. The `forum` library should not alter LMS users during retirement. 2. Changing the email to an empty string results in integrity errors in the MySQL backend, because the email must be unique. --- CHANGELOG.rst | 9 +++++++++ forum/__init__.py | 2 +- forum/api/users.py | 9 +-------- tests/e2e/test_users.py | 7 +++++-- tests/test_views/test_users.py | 16 ++++++++++++---- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ae320716..efd994f2 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -18,6 +18,15 @@ Unreleased * Fix: Set default value for ``pinned`` field on ``CommentThread`` to ``False`` to prevent NULL sort bug. +0.4.4 - 2025-10-21 +****************** + +Fixed +----- + +* Do not modify LMS users during retirement — this is handled by the LMS retirement pipeline. +* Ignore missing users during retirement. + [0.4.0] – 2026-03-12 ********************* diff --git a/forum/__init__.py b/forum/__init__.py index 15fb549a..63ea5f32 100644 --- a/forum/__init__.py +++ b/forum/__init__.py @@ -2,4 +2,4 @@ Openedx forum app. """ -__version__ = "0.4.3" +__version__ = "0.4.4" diff --git a/forum/api/users.py b/forum/api/users.py index 8355f8d3..976591d2 100644 --- a/forum/api/users.py +++ b/forum/api/users.py @@ -135,14 +135,7 @@ def retire_user( 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": [], - }, - ) + backend.update_user(user_id, {"read_states": []}) backend.unsubscribe_all(user_id) backend.retire_all_content(user_id, retired_username) 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..a7618d66 100644 --- a/tests/test_views/test_users.py +++ b/tests/test_views/test_users.py @@ -416,6 +416,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 +427,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 +458,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 +488,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", From ec60bd81a8e2392aef2b19a2b9e1e713ed970661 Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Mon, 13 Jul 2026 22:49:54 +0200 Subject: [PATCH 2/2] fix: ignore non-existent users during retirement If the user does not exist in the forum backend, there is nothing to retire. --- forum/api/users.py | 2 +- tests/test_views/test_users.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/forum/api/users.py b/forum/api/users.py index 976591d2..82c5d716 100644 --- a/forum/api/users.py +++ b/forum/api/users.py @@ -134,7 +134,7 @@ 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}") + 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/tests/test_views/test_users.py b/tests/test_views/test_users.py index a7618d66..dfbc1abd 100644 --- a/tests/test_views/test_users.py +++ b/tests/test_views/test_users.py @@ -393,7 +393,11 @@ def test_attempts_to_retire_user_without_sending_retired_username( 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 +405,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: