diff --git a/lms/djangoapps/course_goals/tests/test_user_activity.py b/lms/djangoapps/course_goals/tests/test_user_activity.py index 55676448b7..d41fca1648 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,7 +173,6 @@ 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 bb7392bd3e..6102948db8 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -1,11 +1,10 @@ """ Discussion API internal interface """ -from __future__ import annotations import itertools from collections import defaultdict -from typing import Dict, List, Literal, Optional, Set, Tuple +from typing import List, Literal, Optional from urllib.parse import urlencode, urlunparse from django.contrib.auth import get_user_model @@ -21,7 +20,6 @@ from rest_framework.request import Request from lms.djangoapps.courseware.courses import get_course_with_access from lms.djangoapps.courseware.exceptions import CourseAccessRedirect 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 @@ -37,7 +35,6 @@ 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, @@ -83,21 +80,11 @@ class DiscussionTopic: Class for discussion topic structure """ - def __init__( - self, - topic_id: Optional[str], - name: str, - thread_list_url: str, - children: Optional[List[DiscussionTopic]] = None, - thread_counts: Dict[str, int] = None, - ): + def __init__(self, topic_id, name, thread_list_url, children=None): self.id = topic_id # pylint: disable=invalid-name self.name = name self.thread_list_url = thread_list_url - 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 + self.children: List[DiscussionTopic] = children or [] # children are of same type i.e. DiscussionTopic class DiscussionEntity(Enum): @@ -261,13 +248,7 @@ def get_course(request, course_key): } -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]]: +def get_courseware_topics(request, course_key, course, topic_ids): """ Returns a list of topic trees for courseware-linked topics. @@ -278,8 +259,6 @@ def get_courseware_topics( 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 @@ -313,8 +292,6 @@ def get_courseware_topics( 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) @@ -327,20 +304,13 @@ def get_courseware_topics( 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: Request, - course_key: CourseKey, - course: CourseBlock, - topic_ids: Optional[List[str]], - thread_counts: Dict[str, Dict[str, int]] -) -> Tuple[List[Dict], Set[str]]: +def get_non_courseware_topics(request, course_key, course, topic_ids): """ Returns a list of topic trees that are not linked to courseware. @@ -351,8 +321,6 @@ def get_non_courseware_topics( 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 @@ -365,9 +333,7 @@ def get_non_courseware_topics( 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"]]), - None, - thread_counts.get(entry["id"]) + entry["id"], name, get_thread_list_url(request, course_key, [entry["id"]]) ) non_courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data) @@ -377,7 +343,7 @@ def get_non_courseware_topics( return non_courseware_topics, existing_topic_ids -def get_course_topics(request: Request, course_key: CourseKey, topic_ids: Optional[Set[str]] = None): +def get_course_topics(request, course_key, topic_ids=None): """ Returns the course topic listing for the given course and user; filtered by 'topic_ids' list if given. @@ -397,13 +363,10 @@ def get_course_topics(request: Request, course_key: CourseKey, topic_ids: Option 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, thread_counts - ) + courseware_topics, existing_courseware_topic_ids = get_courseware_topics(request, course_key, course, topic_ids) non_courseware_topics, existing_non_courseware_topic_ids = get_non_courseware_topics( - request, course_key, course, topic_ids, thread_counts, + request, course_key, course, topic_ids ) if topic_ids: diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 713a1f0492..c0da0bb447 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -499,7 +499,6 @@ 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 d106b05f53..dded8a94f1 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -222,14 +222,10 @@ 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(CommentsServiceMockMixin, ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): +class GetCourseTopicsTest(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( @@ -253,12 +249,6 @@ class GetCourseTopicsTest(CommentsServiceMockMixin, ForumsEnableMixin, UrlResetM 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): """ @@ -296,13 +286,11 @@ class GetCourseTopicsTest(CommentsServiceMockMixin, ForumsEnableMixin, UrlResetM """ 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_counts": thread_counts if not children else None + "thread_list_url": self.get_thread_list_url(topic_id_list) } return node @@ -572,39 +560,29 @@ class GetCourseTopicsTest(CommentsServiceMockMixin, ForumsEnableMixin, UrlResetM 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', - 'thread_counts': {'discussion': 0, 'question': 0}, - }, - ], + {'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'} + ], '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', - 'thread_counts': None, + 'name': 'test_category_1' }, - { - '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}, - } - ], + {'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'} + ], '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', - 'thread_counts': None, + 'name': 'test_category_2' } ] } diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index f8d848e49f..61103af01c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -335,24 +335,13 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, ModuleStoreTestCase): +class CourseTopicsViewTest(DiscussionAPIViewTestMixin, 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): """ @@ -378,7 +367,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, discussion_target=f'Discussion {i}', publish_item=False, ) - return course_url, course.id + return course_url def make_discussion_xblock(self, topic_id, category, subcategory, **kwargs): """ @@ -416,7 +405,6 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, "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}, }], } ) @@ -432,8 +420,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, ) @ddt.unpack def test_bulk_response(self, modules_count, module_store, mongo_calls, topics): - course_url, course_id = self.create_course(modules_count, module_store, topics) - self.register_get_course_commentable_counts_response(course_id, {}) + course_url = self.create_course(modules_count, module_store, topics) with check_mongo_calls(mongo_calls): with modulestore().default_store(module_store): self.client.get(course_url) @@ -474,14 +461,12 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, "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}, + "name": "test_target_1" }], "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", - "thread_counts": None, + "name": "test_category_1" }, { "children": @@ -490,14 +475,12 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, CommentsServiceMockMixin, "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}, + "name": "test_target_2" }], "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", - "thread_counts": None, + "name": "test_category_2" } ] } diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index 29ed62b91f..b062f3c72c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -85,17 +85,6 @@ 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 deleted file mode 100644 index ff3eaddab7..0000000000 --- a/openedx/core/djangoapps/django_comment_common/comment_client/course.py +++ /dev/null @@ -1,41 +0,0 @@ -# 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