From 2b14c3157b4e3e527e6483271bfa054b4a7bb930 Mon Sep 17 00:00:00 2001 From: AsadAzam Date: Fri, 29 Oct 2021 16:20:48 +0500 Subject: [PATCH] Revert "Revert "feat: Add support for returning thread counts for all topics in a course [BD-38] [TNL-8724] [BB-4927] (#29062)" (#29087)" (#29152) This reverts commit ed74db1daf297cdbd0fe7bbeb60f78853534ba7b. --- .../course_goals/tests/test_user_activity.py | 3 +- lms/djangoapps/discussion/rest_api/api.py | 55 +++++++++++++++--- .../discussion/rest_api/serializers.py | 1 + .../discussion/rest_api/tests/test_api.py | 58 +++++++++++++------ .../discussion/rest_api/tests/test_views.py | 31 +++++++--- .../discussion/rest_api/tests/utils.py | 11 ++++ .../comment_client/course.py | 41 +++++++++++++ 7 files changed, 165 insertions(+), 35 deletions(-) create mode 100644 openedx/core/djangoapps/django_comment_common/comment_client/course.py diff --git a/lms/djangoapps/course_goals/tests/test_user_activity.py b/lms/djangoapps/course_goals/tests/test_user_activity.py index d41fca1648..55676448b7 100644 --- a/lms/djangoapps/course_goals/tests/test_user_activity.py +++ b/lms/djangoapps/course_goals/tests/test_user_activity.py @@ -3,7 +3,7 @@ Unit tests for user activity methods. """ from datetime import datetime, timedelta - +from unittest.mock import Mock import ddt from django.contrib.auth import get_user_model @@ -173,6 +173,7 @@ class UserActivityTests(UrlResetMixin, ModuleStoreTestCase): '/api/discussion/v1/courses/{COURSE_ID}/', '/api/discussion/v1/course_topics/{COURSE_ID}', ) + @patch('lms.djangoapps.discussion.rest_api.api.get_course_commentable_counts', Mock(return_value={})) def test_mobile_app_user_activity_calls(self, url): url = url.replace('{COURSE_ID}', str(self.course.id)) with patch.object(UserActivity, 'record_user_activity') as record_user_activity_mock: diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 2433b93bb5..64f612f6a2 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -1,10 +1,11 @@ """ Discussion API internal interface """ +from __future__ import annotations import itertools from collections import defaultdict -from typing import List, Literal, Optional +from typing import Dict, List, Literal, Optional, Set, Tuple from urllib.parse import urlencode, urlunparse from django.contrib.auth import get_user_model @@ -21,6 +22,7 @@ from lms.djangoapps.courseware.courses import get_course_with_access from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from openedx.core.djangoapps.discussions.utils import get_accessible_discussion_xblocks from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment +from openedx.core.djangoapps.django_comment_common.comment_client.course import get_course_commentable_counts 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.models import CourseDiscussionSettings @@ -36,6 +38,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 xmodule.course_module import CourseBlock from .exceptions import ( CommentNotFoundError, DiscussionBlackOutException, @@ -80,11 +83,21 @@ class DiscussionTopic: Class for discussion topic structure """ - def __init__(self, topic_id, name, thread_list_url, children=None): + def __init__( + self, + topic_id: Optional[str], + name: str, + thread_list_url: str, + children: Optional[List[DiscussionTopic]] = None, + thread_counts: Dict[str, int] = None, + ): self.id = topic_id # pylint: disable=invalid-name self.name = name self.thread_list_url = thread_list_url - self.children: List[DiscussionTopic] = children or [] # children are of same type i.e. DiscussionTopic + self.children = children or [] # children are of same type i.e. DiscussionTopic + if not children and not thread_counts: + thread_counts = {"discussion": 0, "question": 0} + self.thread_counts = thread_counts class DiscussionEntity(Enum): @@ -251,7 +264,13 @@ def get_course(request, course_key): } -def get_courseware_topics(request, course_key, course, topic_ids): +def get_courseware_topics( + request: Request, + course_key: CourseKey, + course: CourseBlock, + topic_ids: Optional[List[str]], + thread_counts: Dict[str, Dict[str, int]], +) -> Tuple[List[Dict], Set[str]]: """ Returns a list of topic trees for courseware-linked topics. @@ -262,6 +281,8 @@ def get_courseware_topics(request, course_key, course, topic_ids): course: The course for which topics are requested. topic_ids: A list of topic IDs for which details are requested. This is optional. If None then all course topics are returned. + thread_counts: A map of the thread ids to the count of each type of thread in them + e.g. discussion, question Returns: A list of courseware topics and a set of existing topics among @@ -295,6 +316,8 @@ def get_courseware_topics(request, course_key, course, topic_ids): xblock.discussion_id, xblock.discussion_target, get_thread_list_url(request, course_key, [xblock.discussion_id]), + None, + thread_counts.get(xblock.discussion_id), ) children.append(discussion_topic) @@ -307,13 +330,20 @@ def get_courseware_topics(request, course_key, course, topic_ids): category, get_thread_list_url(request, course_key, [item.discussion_id for item in get_sorted_xblocks(category)]), children, + None, ) courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data) return courseware_topics, existing_topic_ids -def get_non_courseware_topics(request, course_key, course, topic_ids): +def get_non_courseware_topics( + request: Request, + course_key: CourseKey, + course: CourseBlock, + topic_ids: Optional[List[str]], + thread_counts: Dict[str, Dict[str, int]] +) -> Tuple[List[Dict], Set[str]]: """ Returns a list of topic trees that are not linked to courseware. @@ -324,6 +354,8 @@ def get_non_courseware_topics(request, course_key, course, topic_ids): course: The course for which topics are requested. topic_ids: A list of topic IDs for which details are requested. This is optional. If None then all course topics are returned. + thread_counts: A map of the thread ids to the count of each type of thread in them + e.g. discussion, question Returns: A list of non-courseware topics and a set of existing topics among @@ -336,7 +368,9 @@ def get_non_courseware_topics(request, course_key, course, topic_ids): for name, entry in sorted_topics: if not topic_ids or entry['id'] in topic_ids: discussion_topic = DiscussionTopic( - entry["id"], name, get_thread_list_url(request, course_key, [entry["id"]]) + entry["id"], name, get_thread_list_url(request, course_key, [entry["id"]]), + None, + thread_counts.get(entry["id"]) ) non_courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data) @@ -346,7 +380,7 @@ def get_non_courseware_topics(request, course_key, course, topic_ids): return non_courseware_topics, existing_topic_ids -def get_course_topics(request, course_key, topic_ids=None): +def get_course_topics(request: Request, course_key: CourseKey, topic_ids: Optional[Set[str]] = None): """ Returns the course topic listing for the given course and user; filtered by 'topic_ids' list if given. @@ -366,10 +400,13 @@ def get_course_topics(request, course_key, topic_ids=None): DiscussionNotFoundError: If topic/s not found for given topic_ids. """ course = _get_course(course_key, request.user) + thread_counts = get_course_commentable_counts(course.id) - courseware_topics, existing_courseware_topic_ids = get_courseware_topics(request, course_key, course, topic_ids) + courseware_topics, existing_courseware_topic_ids = get_courseware_topics( + request, course_key, course, topic_ids, thread_counts + ) non_courseware_topics, existing_non_courseware_topic_ids = get_non_courseware_topics( - request, course_key, course, topic_ids + request, course_key, course, topic_ids, thread_counts, ) if topic_ids: diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 52786a9c35..c5a0ddbd78 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -497,6 +497,7 @@ class DiscussionTopicSerializer(serializers.Serializer): name = serializers.CharField(read_only=True) thread_list_url = serializers.CharField(read_only=True) children = serializers.SerializerMethodField() + thread_counts = serializers.DictField(read_only=True) def get_children(self, obj): """ diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index dded8a94f1..d106b05f53 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -222,10 +222,14 @@ class GetCourseTestBlackouts(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCa @mock.patch.dict("django.conf.settings.FEATURES", {"DISABLE_START_DATES": False}) @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetCourseTopicsTest(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): +class GetCourseTopicsTest(CommentsServiceMockMixin, ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): """Test for get_course_topics""" @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): + httpretty.reset() + httpretty.enable() + self.addCleanup(httpretty.reset) + self.addCleanup(httpretty.disable) super().setUp() self.maxDiff = None # pylint: disable=invalid-name self.partition = UserPartition( @@ -249,6 +253,12 @@ class GetCourseTopicsTest(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase) self.request = RequestFactory().get("/dummy") self.request.user = self.user CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + self.thread_counts_map = { + "courseware-1": {"discussion": 2, "question": 3}, + "courseware-2": {"discussion": 4, "question": 5}, + "courseware-3": {"discussion": 7, "question": 2}, + } + self.register_get_course_commentable_counts_response(self.course.id, self.thread_counts_map) def make_discussion_xblock(self, topic_id, category, subcategory, **kwargs): """ @@ -286,11 +296,13 @@ class GetCourseTopicsTest(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase) """ topic_id_list = [topic_id] if topic_id else [child["id"] for child in children] children = children or [] + thread_counts = self.thread_counts_map.get(topic_id, {"discussion": 0, "question": 0}) node = { "id": topic_id, "name": name, "children": children, - "thread_list_url": self.get_thread_list_url(topic_id_list) + "thread_list_url": self.get_thread_list_url(topic_id_list), + "thread_counts": thread_counts if not children else None } return node @@ -560,29 +572,39 @@ class GetCourseTopicsTest(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase) assert actual == { 'non_courseware_topics': [], 'courseware_topics': [ - {'children': [ - {'children': [], - 'id': 'topic_id_1', - 'thread_list_url': - 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_1', - 'name': 'test_target_1'} - ], + { + 'children': [ + { + 'children': [], + 'id': 'topic_id_1', + 'thread_list_url': + 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_1', + 'name': 'test_target_1', + 'thread_counts': {'discussion': 0, 'question': 0}, + }, + ], 'id': None, 'thread_list_url': 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_1', - 'name': 'test_category_1' + 'name': 'test_category_1', + 'thread_counts': None, }, - {'children': [ - {'children': [], - 'id': 'topic_id_2', - 'thread_list_url': - 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_2', - 'name': 'test_target_2'} - ], + { + 'children': [ + { + 'children': [], + 'id': 'topic_id_2', + 'thread_list_url': + 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_2', + 'name': 'test_target_2', + 'thread_counts': {'discussion': 0, 'question': 0}, + } + ], 'id': None, 'thread_list_url': 'http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=topic_id_2', - 'name': 'test_category_2' + 'name': 'test_category_2', + 'thread_counts': None, } ] } diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 61103af01c..f8d848e49f 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -335,13 +335,24 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): +class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, ModuleStoreTestCase): """ Tests for CourseTopicsView """ + def setUp(self): + httpretty.reset() + httpretty.enable() + self.addCleanup(httpretty.reset) + self.addCleanup(httpretty.disable) super().setUp() self.url = reverse("course_topics", kwargs={"course_id": str(self.course.id)}) + self.thread_counts_map = { + "courseware-1": {"discussion": 2, "question": 3}, + "courseware-2": {"discussion": 4, "question": 5}, + "courseware-3": {"discussion": 7, "question": 2}, + } + self.register_get_course_commentable_counts_response(self.course.id, self.thread_counts_map) def create_course(self, modules_count, module_store, topics): """ @@ -367,7 +378,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): discussion_target=f'Discussion {i}', publish_item=False, ) - return course_url + return course_url, course.id def make_discussion_xblock(self, topic_id, category, subcategory, **kwargs): """ @@ -405,6 +416,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "children": [], "thread_list_url": "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&topic_id=test_topic", + "thread_counts": {"discussion": 0, "question": 0}, }], } ) @@ -420,7 +432,8 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) @ddt.unpack def test_bulk_response(self, modules_count, module_store, mongo_calls, topics): - course_url = self.create_course(modules_count, module_store, topics) + course_url, course_id = self.create_course(modules_count, module_store, topics) + self.register_get_course_commentable_counts_response(course_id, {}) with check_mongo_calls(mongo_calls): with modulestore().default_store(module_store): self.client.get(course_url) @@ -461,12 +474,14 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "id": "topic_id_1", "thread_list_url": "http://testserver/api/discussion/v1/threads/?" "course_id=x%2Fy%2Fz&topic_id=topic_id_1", - "name": "test_target_1" + "name": "test_target_1", + "thread_counts": {"discussion": 0, "question": 0}, }], "id": None, "thread_list_url": "http://testserver/api/discussion/v1/threads/?" "course_id=x%2Fy%2Fz&topic_id=topic_id_1", - "name": "test_category_1" + "name": "test_category_1", + "thread_counts": None, }, { "children": @@ -475,12 +490,14 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "id": "topic_id_2", "thread_list_url": "http://testserver/api/discussion/v1/threads/?" "course_id=x%2Fy%2Fz&topic_id=topic_id_2", - "name": "test_target_2" + "name": "test_target_2", + "thread_counts": {"discussion": 0, "question": 0}, }], "id": None, "thread_list_url": "http://testserver/api/discussion/v1/threads/?" "course_id=x%2Fy%2Fz&topic_id=topic_id_2", - "name": "test_category_2" + "name": "test_category_2", + "thread_counts": None, } ] } diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index b062f3c72c..29ed62b91f 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -85,6 +85,17 @@ class CommentsServiceMockMixin: status=200 ) + def register_get_course_commentable_counts_response(self, course_id, thread_counts): + """Register a mock response for GET on the CS thread list endpoint""" + assert httpretty.is_enabled(), 'httpretty must be enabled to mock calls.' + + httpretty.register_uri( + httpretty.GET, + f"http://localhost:4567/api/v1/commentables/{course_id}/counts", + body=json.dumps(thread_counts), + status=200 + ) + def register_get_threads_search_response(self, threads, rewrite, num_pages=1): """Register a mock response for GET on the CS thread search endpoint""" assert httpretty.is_enabled(), 'httpretty must be enabled to mock calls.' diff --git a/openedx/core/djangoapps/django_comment_common/comment_client/course.py b/openedx/core/djangoapps/django_comment_common/comment_client/course.py new file mode 100644 index 0000000000..ff3eaddab7 --- /dev/null +++ b/openedx/core/djangoapps/django_comment_common/comment_client/course.py @@ -0,0 +1,41 @@ +# pylint: disable=missing-docstring +"""Provides base Commentable model class""" +from __future__ import annotations + +from typing import Dict + +from opaque_keys.edx.keys import CourseKey + +from openedx.core.djangoapps.django_comment_common.comment_client import settings +from openedx.core.djangoapps.django_comment_common.comment_client.utils import perform_request + + +def get_course_commentable_counts(course_key: CourseKey) -> Dict[str, Dict[str, int]]: + """ + Get stats about the count of different types of threads for each commentable (topic). + + Args: + course_key (str|CourseKey): course key for which stats are needed. + + Returns: + A mapping of topic ids to the number of question and discussion type posts in them. + + e.g. + { + "general": { "discussion": 22, "question": 15 }, + "topic-1": { "discussion": 2, "question": 1 }, + ... + } + + """ + url = f"{settings.PREFIX}/commentables/{course_key}/counts" + response = perform_request( + 'get', + url, + metric_tags=[ + f"course_key:{course_key}", + "function:get_course_commentable_counts", + ], + metric_action='commentable_stats.retrieve', + ) + return response