fix: exclude soft-deleted comments from abuse count when Forum v1 is enabled#157
fix: exclude soft-deleted comments from abuse count when Forum v1 is enabled#157Alam-2U wants to merge 1 commit intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes learner profile “active threads” retrieval to always use the Forum v2 (Python) API so that abuse-flag counts correctly exclude soft-deleted items, avoiding the legacy Ruby service behavior that can incorrectly surface “reported” tags.
Changes:
- Removed waffle-flag-based routing to the legacy
cs_comments_serviceforUser.active_threads. - Normalized/coerced query params (
user_id,page,per_page,count_flagged) before callingforum_api.get_user_active_threads.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not params.get("course_id"): | ||
| params["course_id"] = str(course_key) | ||
| response = forum_api.get_user_active_threads(**params) |
There was a problem hiding this comment.
Since the legacy cs_comments_service branch was removed, callers/tests that previously mocked the /users/{id}/active_threads HTTP endpoint when ENABLE_FORUM_V2 is off will no longer see any request. Please update/add tests to assert the Forum v2 forum_api.get_user_active_threads call even when the course waffle flag is disabled, to prevent regressions in learner profile thread listings.
| course_key = utils.get_course_key(self.attributes.get("course_id")) | ||
| if is_forum_v2_enabled(course_key): | ||
| if user_id := params.get("user_id"): | ||
| params["user_id"] = str(user_id) | ||
| if page := params.get("page"): | ||
| params["page"] = int(page) | ||
| if per_page := params.get("per_page"): | ||
| params["per_page"] = int(per_page) | ||
| if count_flagged := params.get("count_flagged", False): | ||
| params["count_flagged"] = str_to_bool(count_flagged) | ||
| if not params.get("course_id"): | ||
| params["course_id"] = str(course_key) | ||
| response = forum_api.get_user_active_threads(**params) | ||
| else: | ||
| response = utils.perform_request( | ||
| 'get', | ||
| url, | ||
| params, | ||
| metric_action='user.active_threads', | ||
| metric_tags=self._metric_tags, | ||
| paged_results=True, | ||
| ) | ||
| if not params.get("course_id"): | ||
| params["course_id"] = str(course_key) | ||
| response = forum_api.get_user_active_threads(**params) |
There was a problem hiding this comment.
active_threads now always calls Forum v2, but it no longer honors the global override settings.DISABLE_FORUM_V2 (which is_forum_v2_enabled() checked). If an operator has Forum v2 disabled globally, this will still call forum_api.get_user_active_threads and can break environments relying on the legacy service. Consider explicitly checking is_forum_v2_disabled_globally() here and either (a) keep the legacy perform_request fallback or (b) raise a clear CommentClientRequestError explaining Forum v2 is disabled.
| if page := params.get("page"): | ||
| params["page"] = int(page) | ||
| if per_page := params.get("per_page"): | ||
| params["per_page"] = int(per_page) |
There was a problem hiding this comment.
Casting page / per_page with int(...) can raise ValueError for non-numeric query params (e.g., page=abc). Previously, when Forum v2 was disabled, these values were sent through to the legacy service which could return a 4xx; now this can bubble up as a 500 from the LMS. Consider validating these inputs and raising a CommentClientRequestError (or normalizing to defaults) when conversion fails.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| patcher = mock.patch( | ||
| "openedx.core.djangoapps.discussions.config.waffle.ENABLE_FORUM_V2.is_enabled", | ||
| return_value=False, | ||
| return_value=True, |
There was a problem hiding this comment.
This test class still uses CommentsServiceMockMixin/httpretty mocks (e.g., register_get_user_response()), but ENABLE_FORUM_V2.is_enabled is now patched to return True. That will route CommentClientUser.retrieve() to forum_api.get_user() instead of the httpretty-mocked /api/v1/users/... endpoint, so these tests will start making unmocked Forum v2 calls and likely fail. Either keep this patch False for this class, or migrate the class to ForumMockUtilsMixin + setUpClassAndForumMock() and use the forum-api mock helpers.
| return_value=True, | |
| return_value=False, |
| @ddt.ddt | ||
| @httpretty.activate | ||
| @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) | ||
| class LearnerThreadViewAPITest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): | ||
| class LearnerThreadViewAPITest( | ||
| ForumsEnableMixin, | ||
| ForumMockUtilsMixin, | ||
| UrlResetMixin, |
There was a problem hiding this comment.
LearnerThreadView calls forum_api.get_thread(...) inside get_learner_active_thread_list to check deletion status, but this test class no longer has @httpretty.activate and also doesn’t patch lms.djangoapps.discussion.rest_api.api.forum_api. That means the request path will execute the real forum_api.get_thread implementation (network/DB) during tests. Consider explicitly mocking rest_api.api.forum_api.get_thread to return a deterministic payload (e.g., is_deleted=False) so the tests don’t rely on external behavior and remain stable.
| if count_flagged := params.get("count_flagged", False): | ||
| params["count_flagged"] = str_to_bool(count_flagged) | ||
| if not params.get("course_id"): | ||
| params["course_id"] = str(course_key) | ||
| response = forum_api.get_user_active_threads(**params) |
There was a problem hiding this comment.
active_threads() now unconditionally calls forum_api.get_user_active_threads(), which bypasses the global DISABLE_FORUM_V2 kill switch used by is_forum_v2_enabled() (see openedx/core/djangoapps/discussions/config/waffle.py). If ops relies on DISABLE_FORUM_V2 to force a fallback during incidents, this method will still hit Forum v2. Consider explicitly respecting the global override (e.g., guard on is_forum_v2_disabled_globally()), or document that this path intentionally ignores it.
Alam-2U
force-pushed
the
ealam/COSMO2-817
branch
from
b1301e3 to
25e8c25
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return thread | ||
|
|
||
| def test_inactive(self): | ||
| self.user.is_active = False |
There was a problem hiding this comment.
test_inactive sets self.user.is_active = False but never saves the user. Since self.client.get(...) will typically re-load the authenticated user from the database, this test may not actually exercise the inactive-user behavior. Save the user (and/or re-authenticate) after toggling is_active so the request sees the inactive state.
| self.user.is_active = False | |
| self.user.is_active = False | |
| self.user.save() |
| # Always use Forum v2 API (has correct is_deleted filtering for abuse_flagged_count) | ||
| if user_id := params.get("user_id"): | ||
| params["user_id"] = str(user_id) | ||
| else: | ||
| response = utils.perform_request( | ||
| 'get', | ||
| url, | ||
| params, | ||
| metric_action='user.active_threads', | ||
| metric_tags=self._metric_tags, | ||
| paged_results=True, | ||
| ) | ||
| # Use the user's own ID if not provided in query_params |
There was a problem hiding this comment.
active_threads now unconditionally calls forum_api.get_user_active_threads, bypassing both the course waffle flag and the global kill-switch (settings.DISABLE_FORUM_V2 via is_forum_v2_disabled_globally). If Forum v2 is disabled globally (or temporarily taken out of service), this method will still attempt to call it and can break learner profile/thread views. Consider respecting the global disable (and optionally falling back to the legacy request path) while still bypassing only the course-level waffle flag as intended by this change.
Alam-2U
force-pushed
the
ealam/COSMO2-817
branch
from
fdcae23 to
265b3f3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| abuse_count = 0 | ||
| for response_type in ["endorsed_responses", "non_endorsed_responses", "children"]: | ||
| if response_type in forum_thread: | ||
| for child in forum_thread.get(response_type, []): | ||
| if child.get("abuse_flaggers", []) and not child.get("is_deleted", False): | ||
| abuse_count += 1 | ||
| thread["abuse_flagged_count"] = abuse_count |
There was a problem hiding this comment.
The recalculation of abuse_flagged_count doesn’t match the API’s documented semantics (“flags/reports on and within the thread”): it (1) only inspects first-level response lists and ignores nested comments when recursive=False, (2) doesn’t include thread-level flags, and (3) increments by 1 per flagged child rather than counting the actual number of reports/flaggers. This can undercount and potentially clear the “reported” state when only nested comments or the thread itself is flagged, or when multiple users flag the same comment. Consider using Forum v2’s server-provided aggregate count (or computing a recursive sum of len(abuse_flaggers) across the thread + all descendants, excluding is_deleted=True) instead of this partial traversal.
| params = {} | ||
| if count_flagged and "abuse_flagged_count" in thread: | ||
| params = {"with_responses": True, "recursive": False} | ||
|
|
There was a problem hiding this comment.
There’s trailing whitespace on this blank line, which can cause lint failures in repos that enforce whitespace checks. Remove the extra spaces so the line is truly empty.
| # Fetch thread with responses to recalculate abuse_flagged_count if needed | ||
| params = {} | ||
| if count_flagged and "abuse_flagged_count" in thread: | ||
| params = {"with_responses": True, "recursive": False} | ||
|
|
||
| forum_thread = forum_api.get_thread( | ||
| thread.get("id"), course_id=str(course_key) | ||
| thread.get("id"), params=params, course_id=str(course_key) | ||
| ) |
There was a problem hiding this comment.
The PR description says the conditional routing is removed and Forum v2’s get_user_active_threads() is always used. This code still calls comment_client_user.active_threads(...) (which can hit the legacy Ruby service when Forum v2 is disabled) and then tries to patch results per-thread. If the goal is to fully avoid the legacy abuse_flagged_count behavior, this should route the learner threads list through the Forum v2 active-threads API directly instead of relying on legacy output + per-thread fixups.
| # Fetch thread with responses to recalculate abuse_flagged_count if needed | ||
| params = {} | ||
| if count_flagged and "abuse_flagged_count" in thread: | ||
| params = {"with_responses": True, "recursive": False} | ||
|
|
||
| forum_thread = forum_api.get_thread( | ||
| thread.get("id"), course_id=str(course_key) | ||
| thread.get("id"), params=params, course_id=str(course_key) | ||
| ) |
There was a problem hiding this comment.
This introduces an N+1 pattern for the learner profile endpoint: for each thread returned by active_threads, you fetch the full thread again, and when count_flagged is enabled you also request responses (with_responses=True). That can significantly increase latency and payload size for profiles with many threads. Prefer getting the needed fields (deleted status + correct abuse counts) from a single Forum v2 active-threads call or a bulk/thread-list endpoint rather than per-thread get_thread calls.
Alam-2U
force-pushed
the
ealam/COSMO2-817
branch
from
265b3f3 to
ff842e2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _build_comment_client_user(user_id, course_key, group_id): | ||
| if group_id is None: | ||
| return comment_client.User(id=user_id, course_id=course_key) | ||
| return comment_client.User(id=user_id, course_id=course_key, group_id=group_id) | ||
|
|
There was a problem hiding this comment.
All five new private helper functions (_build_comment_client_user, _forum_thread_fetch_params, _augment_deleted_thread_fields, _recalculate_abuse_flagged_count, _filter_active_threads) lack docstrings. Other private helper functions in this file consistently have docstrings (e.g., _get_course at line 192, _get_comment_and_context at line 276, _is_user_author_or_privileged at line 292, _check_fields at line 1520). Adding docstrings would be consistent with the codebase convention and improve maintainability.
| def _recalculate_abuse_flagged_count(thread, forum_thread): | ||
| abuse_count = 0 | ||
| for response_type in ["endorsed_responses", "non_endorsed_responses", "children"]: | ||
| for child in forum_thread.get(response_type, []): | ||
| if child.get("abuse_flaggers", []) and not child.get("is_deleted", False): | ||
| abuse_count += 1 | ||
| thread["abuse_flagged_count"] = abuse_count |
There was a problem hiding this comment.
The _recalculate_abuse_flagged_count function only counts abuse flags on direct children (responses) of the thread. With recursive=False in _forum_thread_fetch_params, nested replies (children of children) are not included in the response, so their abuse flags won't be counted. Additionally, abuse flags on the thread itself are not counted, even though the API documentation for abuse_flagged_count states it represents "The number of flags(reports) on and within the thread" (see views.py line 645).
Consider either:
- Using
recursive=Trueto include nested replies, and also checking the thread's ownabuse_flaggers, or - Documenting this as a known limitation if a partial count is acceptable.
|
|
||
|
|
||
| def _forum_thread_fetch_params(thread, count_flagged): | ||
| if count_flagged and "abuse_flagged_count" in thread: |
There was a problem hiding this comment.
When count_flagged is true and a thread has abuse_flagged_count, _forum_thread_fetch_params requests with_responses=True, which causes the per-thread forum_api.get_thread call to fetch all direct responses. Combined with the existing N+1 pattern (one call per thread in the list), this can be expensive for threads with many responses. Consider whether the abuse_flagged_count could be recalculated server-side in the Forum v2 backend (e.g., via an updated get_user_active_threads that correctly filters deleted items from the count), rather than re-fetching and re-counting on the client side.
| if count_flagged and "abuse_flagged_count" in thread: | |
| if ( | |
| count_flagged | |
| and "abuse_flagged_count" in thread | |
| and thread.get("abuse_flagged_count", 0) > 0 | |
| ): |
Alam-2U
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Alam-2U
changed the title
Alam-2U
force-pushed
the
ealam/COSMO2-817
branch
from
29bd28a to
d9b97a9
Compare
Description
Posts with abuse-flagged soft-deleted comments were incorrectly displaying the "reported" tag in learner profiles when Forum v1 (Ruby service) is enabled.
The Ruby forum service returns an
abuse_flagged_countthat includes deleted comments, which caused posts to appear as reported even when the abuse-flagged comments had already been soft deleted.Solution
Refactored
get_learner_active_thread_listto ensure soft-deleted comments are excluded from abuse flag counts when fetching data from the Ruby forum service.Key Changes
_recalculate_abuse_flagged_countRecalculates
abuse_flagged_countby filtering out comments marked withis_deleted: true._filter_active_threadsApplies deletion filtering and triggers abuse count recalculation.
Ticket
COSMO2-817