From e28fc4c349420bee90bb3afcaa28e6f78420e2ed Mon Sep 17 00:00:00 2001 From: Naincy Chourasia Date: Mon, 6 Jul 2026 05:44:40 +0000 Subject: [PATCH 1/2] fix: inconsistent loading of discussion thread response --- lms/djangoapps/discussion/rest_api/api.py | 100 ++++++--- .../discussion/rest_api/serializers.py | 209 ++++++++++-------- 2 files changed, 188 insertions(+), 121 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 2c44f3293764..6d552436d692 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -22,6 +22,7 @@ from django.urls import reverse from django.utils.html import strip_tags from edx_django_utils.monitoring import function_trace, set_custom_attribute +from forum.backend import get_backend from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey from pytz import UTC @@ -1671,7 +1672,7 @@ def get_comment_list( discussion.rest_api.views.CommentViewSet for more detail. """ response_skip = page_size * (page - 1) - reverse_order = request.GET.get("reverse_order", False) + reverse_order = request.GET.get("reverse_order", "").lower() in ("true", "1") from_mfe_sidebar = request.GET.get("enable_in_context_sidebar", False) cc_thread, context = _get_thread_and_context( request, @@ -2292,74 +2293,107 @@ def get_response_comments(request, comment_id, page, page_size, requested_fields """ try: cc_comment = Comment(id=comment_id).retrieve() - reverse_order = request.GET.get("reverse_order", False) + reverse_order = request.GET.get("reverse_order", "").lower() in ("true", "1") show_deleted = request.GET.get("show_deleted", False) show_deleted = show_deleted in ["true", "True", True] + # Only fetch thread for context/permissions — NOT the full response tree cc_thread, context = _get_thread_and_context( request, cc_comment["thread_id"], retrieve_kwargs={ - "with_responses": True, - "recursive": True, - "reverse_order": reverse_order, - "show_deleted": show_deleted, + "with_responses": False, + "recursive": False, }, ) - if cc_thread["thread_type"] == "question": - thread_responses = itertools.chain( - cc_thread["endorsed_responses"], cc_thread["non_endorsed_responses"] + + if show_deleted and not context["has_moderation_privilege"]: + raise PermissionDenied( + "`show_deleted` can only be set by users with moderation roles." ) - else: - thread_responses = cc_thread["children"] - response_comments = [] - for response in thread_responses: - if response["id"] == comment_id: - response_comments = response["children"] - break - - # Filter deleted content from the FULL list first + + # Determine sort order + sorting_order = -1 if reverse_order else 1 + + # Get the course_id from the thread for backend initialization + course_id = cc_thread["course_id"] + backend = get_backend(course_id)() + + # Fetch ONLY this comment's children directly from the backend + # This avoids loading the entire thread tree (the main performance fix) + response_comments = backend.get_comments( + parent_id=str(comment_id), + depth=1, + sort=sorting_order, + ) + + # Filter deleted content if not show_deleted: response_comments = [ - response - for response in response_comments - if not response.get("is_deleted", False) + comment for comment in response_comments + if not comment.get("is_deleted", False) ] - else: - if not context["has_moderation_privilege"]: - raise PermissionDenied( - "`show_deleted` can only be set by users with moderation roles." - ) - # Filter muted content from the FULL list - include_muted = request.GET.get("include_muted", False) - include_muted = include_muted in ["true", "True", True] - if not include_muted: + # Filter muted content + include_muted_param = request.GET.get("include_muted", False) + include_muted_param = include_muted_param in ["true", "True", True] + if not include_muted_param: response_comments = filter_muted_content( request.user, context["course"].id, response_comments ) - # NOW calculate pagination based on FILTERED total + # Calculate pagination based on FILTERED total total_comments_count = len(response_comments) num_pages = ( (total_comments_count + page_size - 1) // page_size if total_comments_count else 1 ) - # Then paginate the filtered list + # Paginate the filtered list response_skip = page_size * (page - 1) paged_response_comments = response_comments[ response_skip: (response_skip + page_size) ] + if not paged_response_comments and page != 1: raise PageNotFoundError("Page not found (No results on this page).") + # Normalize comment data for serialization (backend returns raw dict format) + normalized_comments = [] + for comment in paged_response_comments: + normalized = { + "id": str(comment.get("_id", comment.get("id", ""))), + "body": comment.get("body", ""), + "course_id": comment.get("course_id", course_id), + "user_id": comment.get("author_id", comment.get("user_id")), + "username": comment.get("author_username", comment.get("username", "")), + "thread_id": str(comment.get("comment_thread_id", comment.get("thread_id", ""))), + "parent_id": str(comment.get("parent_id", "")) if comment.get("parent_id") else None, + "created_at": comment.get("created_at"), + "updated_at": comment.get("updated_at"), + "depth": comment.get("depth", 1), + "type": "comment", + "anonymous": comment.get("anonymous", False), + "anonymous_to_peers": comment.get("anonymous_to_peers", False), + "endorsed": comment.get("endorsed", False), + "abuse_flaggers": comment.get("abuse_flaggers", []), + "votes": comment.get("votes", {"up_count": 0}), + "child_count": comment.get("child_count", 0), + "children": [], + "closed": comment.get("closed", False), + } + # Carry over any additional fields + for key in ("endorsement", "edit_history", "is_deleted", "deleted_at", "deleted_by"): + if key in comment: + normalized[key] = comment[key] + normalized_comments.append(normalized) + results = _serialize_discussion_entities( request, context, - paged_response_comments, + normalized_comments, requested_fields, DiscussionEntity.comment, ) diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 8e0478e7e03c..e38efa494d92 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -247,23 +247,53 @@ def _validate_non_updatable(self, value): raise ValidationError("This field is not allowed in an update.") return value + def _resolve_user(self, user_id): + """ + Resolve a user_id to a User object using prefetched data, per-request cache, or DB. + Returns User or None. All serializer methods should use this instead of + User.objects.get() directly. + """ + uid = str(user_id) if user_id else None + if not uid: + return None + + # 1. Check prefetched users + prefetched = self.context.get("_prefetched_users") + if prefetched is not None: + user = prefetched.get(uid) + if user is not None: + return user + + # 2. Per-request cache + cache = self.context.setdefault("_user_cache", {}) + if uid in cache: + return cache[uid] + + # 3. DB lookup + try: + user = User.objects.get(id=uid) + cache[uid] = user + return user + except (User.DoesNotExist, ValueError, TypeError): + cache[uid] = None + return None + def _is_user_privileged(self, user_id): """ Returns a boolean indicating whether the given user_id identifies a privileged user. """ - is_privileged = ( + if ( user_id in self.context["moderator_user_ids"] or user_id in self.context["ta_user_ids"] - ) + ): + return True - if not is_privileged: - try: - user = User.objects.get(id=user_id) - is_privileged = GlobalStaff().has_user(user) - except User.DoesNotExist: - pass + prefetched_gs = self.context.get("_prefetched_global_staff_ids") + if prefetched_gs is not None: + return user_id in prefetched_gs - return is_privileged + user = self._resolve_user(user_id) + return GlobalStaff().has_user(user) if user else False def _is_content_anonymous(self, obj): """ @@ -306,9 +336,14 @@ def _is_anonymous(self, obj): def get_author(self, obj): """ Returns the author's username, or None if the content is anonymous to the viewer. - For anonymous_to_peers posts, staff/moderators/admins can see the author. """ - return None if self._is_anonymous(obj) else obj["username"] + if self._is_anonymous(obj): + return None + username = obj.get("username") + if username: + return username + user = self._resolve_user(obj.get("user_id")) + return user.username if user else None def get_author_id(self, obj): """ @@ -334,57 +369,60 @@ def get_author_id(self, obj): def _get_user_label(self, user_id): """ Returns a single legacy role label for the user. - Used by edit_by_label, closed_by_label, endorsed_by_label, deleted_by_label - to preserve backward compatibility. - Returns one of: "Staff", "Administrator", "Moderator", "Community TA", or None. """ is_moderator = user_id in self.context["moderator_user_ids"] is_ta = user_id in self.context["ta_user_ids"] is_global_staff = False if not (is_moderator or is_ta): - try: - user = User.objects.get(id=user_id) - is_global_staff = GlobalStaff().has_user(user) - except User.DoesNotExist: - pass + prefetched_gs = self.context.get("_prefetched_global_staff_ids") + if prefetched_gs is not None: + is_global_staff = user_id in prefetched_gs + else: + user = self._resolve_user(user_id) + is_global_staff = GlobalStaff().has_user(user) if user else False is_administrator = False if is_moderator: - course_id = self.context.get("course_id") - if course_id: - user_roles = Role.objects.filter( - users__id=user_id, - course_id=course_id, - name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], - ).values_list("name", flat=True) - is_administrator = FORUM_ROLE_ADMINISTRATOR in user_roles - + prefetched_roles = self.context.get("_prefetched_roles") + if prefetched_roles is not None: + is_administrator = FORUM_ROLE_ADMINISTRATOR in prefetched_roles.get(user_id, set()) + else: + course_id = self.context.get("course_id") + if course_id: + is_administrator = FORUM_ROLE_ADMINISTRATOR in set( + Role.objects.filter( + users__id=user_id, + course_id=course_id, + name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], + ).values_list("name", flat=True) + ) return ( - "Staff" - if is_global_staff - else "Administrator" - if is_administrator - else "Moderator" if is_moderator else "Community TA" if is_ta else None + "Staff" if is_global_staff + else "Administrator" if is_administrator + else "Moderator" if is_moderator + else "Community TA" if is_ta + else None ) def _get_user_labels_all(self, user_id): """ Returns an array of ALL roles assigned to the user. - Used exclusively by get_author_labels to support multi-role display. - Examples: ["Global Staff", "Course Staff"], ["Administrator", "Community TA"] """ roles = [] - - # Check GlobalStaff (platform-wide) - try: - user = User.objects.get(id=user_id) - if GlobalStaff().has_user(user): + prefetched_gs = self.context.get("_prefetched_global_staff_ids") + prefetched_roles = self.context.get("_prefetched_roles") + user = self._resolve_user(user_id) + + # GlobalStaff + if user: + if prefetched_gs is not None: + if user_id in prefetched_gs: + roles.append("Global Staff") + elif GlobalStaff().has_user(user): roles.append("Global Staff") - except User.DoesNotExist: - user = None - # Check CourseStaff and CourseInstructor (platform course roles) + # CourseStaff / CourseInstructor if user and user_id in self.context.get("course_staff_user_ids", []): course_id = self.context.get("course_id") if course_id: @@ -393,35 +431,41 @@ def _get_user_labels_all(self, user_id): if CourseStaffRole(course_id).has_user(user): roles.append("Course Staff") - # Check discussion-specific moderator roles + # Discussion moderator roles if user_id in self.context.get("moderator_user_ids", []): - course_id = self.context.get("course_id") - if course_id: - user_roles = Role.objects.filter( - users__id=user_id, - course_id=course_id, - name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR] - ).values_list('name', flat=True) - - if FORUM_ROLE_ADMINISTRATOR in user_roles: - roles.append("Administrator") - if FORUM_ROLE_MODERATOR in user_roles: - roles.append("Moderator") - - # Check discussion-specific TA roles + if prefetched_roles is not None: + user_role_names = prefetched_roles.get(user_id, set()) + else: + course_id = self.context.get("course_id") + user_role_names = set( + Role.objects.filter( + users__id=user_id, course_id=course_id, + name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], + ).values_list("name", flat=True) + ) if course_id else set() + + if FORUM_ROLE_ADMINISTRATOR in user_role_names: + roles.append("Administrator") + if FORUM_ROLE_MODERATOR in user_role_names: + roles.append("Moderator") + + # Discussion TA roles if user_id in self.context.get("ta_user_ids", []): - course_id = self.context.get("course_id") - if course_id: - user_roles = Role.objects.filter( - users__id=user_id, - course_id=course_id, - name__in=[FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR] - ).values_list('name', flat=True) - - if FORUM_ROLE_COMMUNITY_TA in user_roles: - roles.append("Community TA") - if FORUM_ROLE_GROUP_MODERATOR in user_roles: - roles.append("Group Moderator") + if prefetched_roles is not None: + user_role_names = prefetched_roles.get(user_id, set()) + else: + course_id = self.context.get("course_id") + user_role_names = set( + Role.objects.filter( + users__id=user_id, course_id=course_id, + name__in=[FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR], + ).values_list("name", flat=True) + ) if course_id else set() + + if FORUM_ROLE_COMMUNITY_TA in user_role_names: + roles.append("Community TA") + if FORUM_ROLE_GROUP_MODERATOR in user_role_names: + roles.append("Group Moderator") return roles if roles else None @@ -442,7 +486,7 @@ def get_author_label(self, obj): Returns None for posts that are anonymous to the viewer. For anonymous_to_peers posts, staff/moderators/admins can see the label. """ - if self._is_anonymous(obj) or obj["user_id"] is None: + if self._is_anonymous(obj) or obj.get("user_id") is None: return None return self._get_user_label(int(obj["user_id"])) @@ -454,7 +498,7 @@ def get_author_labels(self, obj): deleted_by_label) are unaffected and continue to use _get_user_label. Returns None for anonymous posts or users with no recognized roles. """ - if self._is_anonymous(obj) or obj["user_id"] is None: + if self._is_anonymous(obj) or obj.get("user_id") is None: return None return self._get_user_labels_all(int(obj["user_id"])) @@ -463,20 +507,15 @@ def get_learner_status(self, obj): Get the learner status for the discussion post author. Returns one of: "anonymous", "staff", "new", "regular" """ - # Skip for anonymous content if self._is_anonymous(obj) or obj.get("user_id") is None: return "anonymous" - try: - user = User.objects.get(id=int(obj["user_id"])) - except (User.DoesNotExist, ValueError): + user = self._resolve_user(obj["user_id"]) + if not user: return "anonymous" course = self.context.get("course") - if not course: - return "anonymous" - - return get_user_learner_status(user, course.id) + return get_user_learner_status(user, course.id) if course else "anonymous" def get_rendered_body(self, obj): """ @@ -609,14 +648,8 @@ def _get_author_ban_cache_key(self, course_id, user_id): return (str(course_id), int(user_id)) def _get_author_from_cache(self, user_id): - """Fetch author from per-request cache or database.""" - user_cache = self.context.setdefault("_author_ban_user_cache", {}) - if user_id not in user_cache: - try: - user_cache[user_id] = User.objects.get(id=user_id) - except User.DoesNotExist: - user_cache[user_id] = None - return user_cache[user_id] + """Fetch author from cache or database.""" + return self._resolve_user(user_id) def get_is_author_banned(self, obj): """ From 64a5f6ab09a10b1f4d4c8a7a514ed16bf150d4d5 Mon Sep 17 00:00:00 2001 From: Naincy Chourasia Date: Mon, 6 Jul 2026 05:47:47 +0000 Subject: [PATCH 2/2] fix: inconsistent loading of discussion thread response --- lms/djangoapps/discussion/rest_api/api.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 6d552436d692..227c15058117 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -5,7 +5,6 @@ from __future__ import annotations -import itertools import logging import re from collections import defaultdict