From dcb01d107f14438d04c350f1616f002c0c334ecd Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Fri, 12 Aug 2022 17:24:22 +0500 Subject: [PATCH] fix: Temporary fix for learners stats api performance issue (#30847) * fix: Temporary fix for learners' stats API performance issue * fix: resolved linter errors * fix: learners stats API response is now null * fix: changed waffle dates * fix: resolved unit test issue --- lms/djangoapps/discussion/config/waffle.py | 19 +++ lms/djangoapps/discussion/rest_api/api.py | 117 +++++++++++++++--- .../discussion/rest_api/tests/test_views.py | 4 + lms/djangoapps/discussion/rest_api/utils.py | 25 ++++ 4 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 lms/djangoapps/discussion/config/waffle.py diff --git a/lms/djangoapps/discussion/config/waffle.py b/lms/djangoapps/discussion/config/waffle.py new file mode 100644 index 0000000000..e9fc3aa98a --- /dev/null +++ b/lms/djangoapps/discussion/config/waffle.py @@ -0,0 +1,19 @@ +""" +This module contains configuration settings via waffle switches for the discussions. +""" + +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag + +WAFFLE_NAMESPACE = 'discussions' + +# .. toggle_name: discussions.enable_learners_stats +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag to enable learners stats +# .. toggle_use_cases: temporary, open_edx +# .. toggle_creation_date: 2022-08-12 +# .. toggle_target_removal_date: 2022-10-02 +# .. toggle_warning: When the flag is ON, API will return learners stats with original values. +# .. This is temporary fix for performance issue in API. +# .. toggle_tickets: INF-444 +ENABLE_LEARNERS_STATS = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_learners_stats', __name__) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 9e56c7a32a..d249f60df4 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -5,6 +5,7 @@ from __future__ import annotations import itertools from collections import defaultdict + from enum import Enum from typing import Dict, Iterable, List, Literal, Optional, Set, Tuple from urllib.parse import urlencode, urlunparse @@ -42,7 +43,8 @@ from openedx.core.djangoapps.django_comment_common.comment_client.course import get_course_user_stats, ) from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread -from openedx.core.djangoapps.django_comment_common.comment_client.utils import CommentClientRequestError +from openedx.core.djangoapps.django_comment_common.comment_client.utils import CommentClientRequestError, \ + CommentClient500Error from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_COMMUNITY_TA, @@ -64,6 +66,7 @@ from openedx.core.djangoapps.django_comment_common.signals import ( ) from openedx.core.djangoapps.user_api.accounts.api import get_account_settings from openedx.core.lib.exceptions import CourseNotFoundError, DiscussionNotFoundError, PageNotFoundError +from ..config.waffle import ENABLE_LEARNERS_STATS from ..django_comment_client.base.views import ( track_comment_created_event, @@ -101,7 +104,7 @@ from .utils import ( discussion_open_for_user, get_usernames_from_search_string, add_stats_for_users_with_no_discussion_content, - set_attribute, + set_attribute, get_usernames_for_course, ) User = get_user_model() @@ -967,20 +970,29 @@ def get_learner_active_thread_list(request, course_key, query_params): else: comment_client_user = comment_client.User(id=user_id, course_id=course_key, group_id=group_id) - threads, page, num_pages = comment_client_user.active_threads(query_params) - threads = set_attribute(threads, "pinned", False) - results = _serialize_discussion_entities( - request, context, threads, {'profile_image'}, DiscussionEntity.thread - ) - paginator = DiscussionAPIPagination( - request, - page, - num_pages, - len(threads) - ) - return paginator.get_paginated_response({ - "results": results, - }) + try: + threads, page, num_pages = comment_client_user.active_threads(query_params) + threads = set_attribute(threads, "pinned", False) + results = _serialize_discussion_entities( + request, context, threads, {'profile_image'}, DiscussionEntity.thread + ) + paginator = DiscussionAPIPagination( + request, + page, + num_pages, + len(threads) + ) + return paginator.get_paginated_response({ + "results": results, + }) + except CommentClient500Error: + return DiscussionAPIPagination( + request, + page_num=1, + num_pages=0, + ).get_paginated_response({ + "results": [], + }) def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=False, requested_fields=None): @@ -1639,6 +1651,17 @@ def get_course_discussion_user_stats( order_by = order_by or UserOrdering.BY_ACTIVITY if order_by != UserOrdering.BY_ACTIVITY: raise ValidationError({"order_by": "Invalid value"}) + + if not ENABLE_LEARNERS_STATS.is_enabled(course_key): + return get_users_without_stats( + username_search_string, + course_key, + page, + page_size, + request, + is_privileged + ) + params = { 'sort_key': str(order_by), 'page': page, @@ -1679,3 +1702,65 @@ def get_course_discussion_user_stats( return paginator.get_paginated_response({ "results": serializer.data, }) + + +def get_users_without_stats( + username_search_string, + course_key, + page_number, + page_size, + request, + is_privileged +): + """ + This return users with no user stats. + This function will be deprecated when this ticket DOS-3414 is resolved + """ + if username_search_string: + comma_separated_usernames, matched_users_count, matched_users_pages = get_usernames_from_search_string( + course_key, username_search_string, page_number, page_size + ) + if not comma_separated_usernames: + return DiscussionAPIPagination(request, 0, 1).get_paginated_response({ + "results": [], + }) + + else: + comma_separated_usernames, matched_users_count, matched_users_pages = get_usernames_for_course( + course_key, page_number, page_size + ) + + if comma_separated_usernames: + updated_course_stats = add_stats_for_users_with_null_values([], comma_separated_usernames) + + serializer = UserStatsSerializer(updated_course_stats, context={"is_privileged": is_privileged}, many=True) + paginator = DiscussionAPIPagination( + request, + page_number, + matched_users_pages, + matched_users_count, + ) + return paginator.get_paginated_response({ + "results": serializer.data, + }) + + +def add_stats_for_users_with_null_values(course_stats, users_in_course): + """ + Update users stats for users with no discussion stats available in course + """ + users_returned_from_api = [user['username'] for user in course_stats] + user_list = users_in_course.split(',') + users_with_no_discussion_content = set(user_list) ^ set(users_returned_from_api) + updated_course_stats = course_stats + for user in users_with_no_discussion_content: + updated_course_stats.append({ + 'username': user, + 'threads': None, + 'replies': None, + 'responses': None, + 'active_flags': None, + 'inactive_flags': None, + }) + updated_course_stats = sorted(updated_course_stats, key=lambda d: len(d['username'])) + return updated_course_stats diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 53870dc7ae..34896b5932 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -14,11 +14,14 @@ import httpretty from django.core.files.uploadedfile import SimpleUploadedFile from django.test import override_settings from django.urls import reverse +from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.keys import CourseKey from pytz import UTC from rest_framework import status from rest_framework.parsers import JSONParser from rest_framework.test import APIClient, APITestCase + +from lms.djangoapps.discussion.config.waffle import ENABLE_LEARNERS_STATS from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase @@ -2890,6 +2893,7 @@ class CourseDiscussionRolesAPIViewTest(APITestCase, UrlResetMixin, ModuleStoreTe @ddt.ddt @httpretty.activate +@override_waffle_flag(ENABLE_LEARNERS_STATS, True) class CourseActivityStatsTest(ForumsEnableMixin, UrlResetMixin, CommentsServiceMockMixin, APITestCase, SharedModuleStoreTestCase): """ diff --git a/lms/djangoapps/discussion/rest_api/utils.py b/lms/djangoapps/discussion/rest_api/utils.py index b2ad505ba5..8f9b8c48e6 100644 --- a/lms/djangoapps/discussion/rest_api/utils.py +++ b/lms/djangoapps/discussion/rest_api/utils.py @@ -69,6 +69,31 @@ def get_usernames_from_search_string(course_id, search_string, page_number, page return ','.join(page_matched_users), matched_users_count, matched_users_pages +def get_usernames_for_course(course_id, page_number, page_size): + """ + Gets usernames for all users in course. + + Args: + course_id (CourseKey): Course to check discussions for + page_number (int): Page numbers to fetch + page_size (int): Number of items in each page + + Returns: + page_matched_users (str): comma seperated usernames for the page + matched_users_count (int): count of matched users in course + matched_users_pages (int): pages of matched users in course + """ + matched_users_in_course = User.objects.filter(courseenrollment__course_id=course_id,)\ + .order_by(Length('username').asc()).values_list('username', flat=True) + if not matched_users_in_course: + return '', 0, 0 + matched_users_count = len(matched_users_in_course) + paginator = Paginator(matched_users_in_course, page_size) + page_matched_users = paginator.page(page_number) + matched_users_pages = int(matched_users_count / page_size) + return ','.join(page_matched_users), matched_users_count, matched_users_pages + + def add_stats_for_users_with_no_discussion_content(course_stats, users_in_course): """ Update users stats for users with no discussion stats available in course