Merge pull request #12445 from edx/jia/MA-2318

MA-2318: get DiscussionTopic against topic id

lms/djangoapps/discussion_api/api.py

@@ -24,7 +24,7 @@ from discussion_api.permissions import (
     get_initializable_comment_fields,
     get_initializable_thread_fields,
 )
-from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context
+from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context, DiscussionTopicSerializer
 from django_comment_client.base.views import (
     track_comment_created_event,
     track_thread_created_event,
@@ -46,7 +46,19 @@ from lms.lib.comment_client.comment import Comment
 from lms.lib.comment_client.thread import Thread
 from lms.lib.comment_client.utils import CommentClientRequestError
 from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id
-from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError
+from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError, DiscussionNotFoundError
+
+
+class DiscussionTopic(object):
+    """
+    Class for discussion topic structure
+    """
+
+    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
 
 
 def _get_course(course_key, user):
@@ -173,70 +185,132 @@ def get_course(request, course_key):
     }
 
 
-def get_course_topics(request, course_key):
+def get_courseware_topics(request, course_key, course, topic_ids):
     """
-    Return the course topic listing for the given course and user.
+    Returns a list of topic trees for courseware-linked topics.
 
     Parameters:
 
-    course_key: The key of the course to get topics for
-    user: The requesting user, for access control
+        request: The django request objects used for build_absolute_uri.
+        course_key: The key of the course to get discussion threads for.
+        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.
 
     Returns:
+        A list of courseware topics and a set of existing topics among
+        topic_ids.
 
-    A course topic listing dictionary; see discussion_api.views.CourseTopicViews
-    for more detail.
     """
+    courseware_topics = []
+    existing_topic_ids = set()
+
     def get_module_sort_key(module):
         """
         Get the sort key for the module (falling back to the discussion_target
         setting if absent)
         """
         return module.sort_key or module.discussion_target
-    course = _get_course(course_key, request.user)
+
+    def get_sorted_modules(category):
+        """Returns key sorted modules by category"""
+        return sorted(modules_by_category[category], key=get_module_sort_key)
+
     discussion_modules = get_accessible_discussion_modules(course, request.user)
     modules_by_category = defaultdict(list)
     for module in discussion_modules:
         modules_by_category[module.discussion_category].append(module)
 
-    def get_sorted_modules(category):
-        """Returns key sorted modules by category"""
-        return sorted(modules_by_category[category], key=get_module_sort_key)
+    for category in sorted(modules_by_category.keys()):
+        children = []
+        for module in get_sorted_modules(category):
+            if not topic_ids or module.discussion_id in topic_ids:
+                discussion_topic = DiscussionTopic(
+                    module.discussion_id,
+                    module.discussion_target,
+                    get_thread_list_url(request, course_key, [module.discussion_id]),
+                )
+                children.append(discussion_topic)
+
+                if topic_ids and module.discussion_id in topic_ids:
+                    existing_topic_ids.add(module.discussion_id)
+
+        if not topic_ids or children:
+            discussion_topic = DiscussionTopic(
+                None,
+                category,
+                get_thread_list_url(request, course_key, [item.discussion_id for item in get_sorted_modules(category)]),
+                children,
+            )
+            courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data)
 
-    courseware_topics = [
-        {
-            "id": None,
-            "name": category,
-            "thread_list_url": get_thread_list_url(
-                request,
-                course_key,
-                [item.discussion_id for item in get_sorted_modules(category)]
-            ),
-            "children": [
-                {
-                    "id": module.discussion_id,
-                    "name": module.discussion_target,
-                    "thread_list_url": get_thread_list_url(request, course_key, [module.discussion_id]),
-                    "children": [],
-                }
-                for module in get_sorted_modules(category)
-            ],
-        }
-        for category in sorted(modules_by_category.keys())
-    ]
-
-    non_courseware_topics = [
-        {
-            "id": entry["id"],
-            "name": name,
-            "thread_list_url": get_thread_list_url(request, course_key, [entry["id"]]),
-            "children": [],
-        }
-        for name, entry in sorted(
-            course.discussion_topics.items(),
-            key=lambda item: item[1].get("sort_key", item[0])
-        )
-    ]
+    return courseware_topics, existing_topic_ids
+
+
+def get_non_courseware_topics(request, course_key, course, topic_ids):
+    """
+    Returns a list of topic trees that are not linked to courseware.
+
+    Parameters:
+
+        request: The django request objects used for build_absolute_uri.
+        course_key: The key of the course to get discussion threads for.
+        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.
+
+    Returns:
+        A list of non-courseware topics and a set of existing topics among
+        topic_ids.
+
+    """
+    non_courseware_topics = []
+    existing_topic_ids = set()
+    for name, entry in sorted(course.discussion_topics.items(), key=lambda item: item[1].get("sort_key", item[0])):
+        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"]])
+            )
+            non_courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data)
+
+            if topic_ids and entry["id"] in topic_ids:
+                existing_topic_ids.add(entry["id"])
+
+    return non_courseware_topics, existing_topic_ids
+
+
+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.
+
+    Parameters:
+
+        course_key: The key of the course to get topics for
+        user: The requesting user, for access control
+        topic_ids: A list of topic IDs for which topic details are requested
+
+    Returns:
+
+        A course topic listing dictionary; see discussion_api.views.CourseTopicViews
+        for more detail.
+
+    Raises:
+        DiscussionNotFoundError: If topic/s not found for given topic_ids.
+    """
+    course = _get_course(course_key, request.user)
+
+    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
+    )
+
+    if topic_ids:
+        not_found_topic_ids = topic_ids - (existing_courseware_topic_ids | existing_non_courseware_topic_ids)
+        if not_found_topic_ids:
+            raise DiscussionNotFoundError(
+                "Discussion not found for '{}'.".format(", ".join(str(id) for id in not_found_topic_ids))
+            )
 
     return {
         "courseware_topics": courseware_topics,

lms/djangoapps/discussion_api/serializers.py

@@ -395,3 +395,33 @@ class CommentSerializer(_ContentSerializer):
 
         instance.save()
         return instance
+
+
+class DiscussionTopicSerializer(serializers.Serializer):
+    """
+    Serializer for DiscussionTopic
+    """
+    id = serializers.CharField(read_only=True)  # pylint: disable=invalid-name
+    name = serializers.CharField(read_only=True)
+    thread_list_url = serializers.CharField(read_only=True)
+    children = serializers.SerializerMethodField()
+
+    def get_children(self, obj):
+        """
+        Returns a list of children of DiscussionTopicSerializer type
+        """
+        if not obj.children:
+            return []
+        return [DiscussionTopicSerializer(child).data for child in obj.children]
+
+    def create(self, validated_data):
+        """
+        Overriden create abstract method
+        """
+        pass
+
+    def update(self, instance, validated_data):
+        """
+        Overriden update abstract method
+        """
+        pass

lms/djangoapps/discussion_api/tests/test_api.py

@@ -483,6 +483,51 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase):
         }
         self.assertEqual(staff_actual, staff_expected)
 
+    def test_discussion_topic(self):
+        """
+        Tests discussion topic details against a requested topic id
+        """
+        topic_id_1 = "topic_id_1"
+        topic_id_2 = "topic_id_2"
+        self.make_discussion_module(topic_id_1, "test_category_1", "test_target_1")
+        self.make_discussion_module(topic_id_2, "test_category_2", "test_target_2")
+        actual = get_course_topics(self.request, self.course.id, {"topic_id_1", "topic_id_2"})
+        self.assertEqual(
+            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"
+                        }],
+                        "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"
+                    },
+                    {
+                        "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"
+                    }
+                ]
+            }
+        )
+
 
 @attr('shard_2')
 @ddt.ddt

lms/djangoapps/discussion_api/tests/test_views.py

@@ -151,7 +151,9 @@ class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
 @ddt.ddt
 @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
 class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
-    """Tests for CourseTopicsView"""
+    """
+    Tests for CourseTopicsView
+    """
     def setUp(self):
         super(CourseTopicsViewTest, self).setUp()
         self.url = reverse("course_topics", kwargs={"course_id": unicode(self.course.id)})
@@ -182,6 +184,19 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             )
         return course_url
 
+    def make_discussion_module(self, topic_id, category, subcategory, **kwargs):
+        """
+        Build a discussion module in self.course
+        """
+        ItemFactory.create(
+            parent_location=self.course.location,
+            category="discussion",
+            discussion_id=topic_id,
+            discussion_category=category,
+            discussion_target=subcategory,
+            **kwargs
+        )
+
     def test_404(self):
         response = self.client.get(
             reverse("course_topics", kwargs={"course_id": "non/existent/course"})
@@ -225,6 +240,67 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             with modulestore().default_store(module_store):
                 self.client.get(course_url)
 
+    def test_discussion_topic_404(self):
+        """
+        Tests discussion topic does not exist for the given topic id.
+        """
+        topic_id = "courseware-topic-id"
+        self.make_discussion_module(topic_id, "test_category", "test_target")
+        url = "{}?topic_id=invalid_topic_id".format(self.url)
+        response = self.client.get(url)
+        self.assert_response_correct(
+            response,
+            404,
+            {"developer_message": "Discussion not found for 'invalid_topic_id'."}
+        )
+
+    def test_topic_id(self):
+        """
+        Tests discussion topic details against a requested topic id
+        """
+        topic_id_1 = "topic_id_1"
+        topic_id_2 = "topic_id_2"
+        self.make_discussion_module(topic_id_1, "test_category_1", "test_target_1")
+        self.make_discussion_module(topic_id_2, "test_category_2", "test_target_2")
+        url = "{}?topic_id=topic_id_1,topic_id_2".format(self.url)
+        response = self.client.get(url)
+        self.assert_response_correct(
+            response,
+            200,
+            {
+                "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"
+                        }],
+                        "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"
+                    },
+                    {
+                        "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"
+                    }
+                ]
+            }
+        )
+
 
 @attr('shard_3')
 @ddt.ddt

lms/djangoapps/discussion_api/views.py

@@ -75,11 +75,11 @@ class CourseTopicsView(DeveloperErrorViewMixin, APIView):
     **Example Requests**:
 
         GET /api/discussion/v1/course_topics/course-v1:ExampleX+Subject101+2015
+            ?topic_id={topic_id_1, topid_id_2}
 
     **Response Values**:
-
         * courseware_topics: The list of topic trees for courseware-linked
-          topics. Each item in the list includes:
+            topics. Each item in the list includes:
 
             * id: The id of the discussion topic (null for a topic that only
               has children but cannot contain threads itself).
@@ -92,10 +92,17 @@ class CourseTopicsView(DeveloperErrorViewMixin, APIView):
               courseware. Items are of the same format as in courseware_topics.
     """
     def get(self, request, course_id):
-        """Implements the GET method as described in the class docstring."""
+        """
+        Implements the GET method as described in the class docstring.
+        """
         course_key = CourseKey.from_string(course_id)
+        topic_ids = self.request.GET.get('topic_id')
         with modulestore().bulk_operations(course_key):
-            response = get_course_topics(request, course_key)
+            response = get_course_topics(
+                request,
+                course_key,
+                set(topic_ids.strip(',').split(',')) if topic_ids else None,
+            )
         return Response(response)
 
 

lms/djangoapps/django_comment_client/utils.py

@@ -385,7 +385,7 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude
     return _filter_unstarted_categories(category_map, course) if exclude_unstarted else category_map
 
 
-def discussion_category_id_access(course, user, discussion_id):
+def discussion_category_id_access(course, user, discussion_id, module=None):
     """
     Returns True iff the given discussion_id is accessible for user in course.
     Assumes that the commentable identified by discussion_id has a null or 'course' context.
@@ -395,10 +395,11 @@ def discussion_category_id_access(course, user, discussion_id):
     if discussion_id in course.top_level_discussion_topic_ids:
         return True
     try:
-        key = get_cached_discussion_key(course, discussion_id)
-        if not key:
-            return False
-        module = modulestore().get_item(key)
+        if not module:
+            key = get_cached_discussion_key(course, discussion_id)
+            if not key:
+                return False
+            module = modulestore().get_item(key)
         return has_required_keys(module) and has_access(user, 'load', module, course.id)
     except DiscussionIdMapIsNotCached:
         return discussion_id in get_discussion_categories_ids(course, user)

openedx/core/lib/exceptions.py

-""" Common Purpose Errors"""
+"""
+Common Purpose Errors
+"""
 from django.core.exceptions import ObjectDoesNotExist
 
 
 class CourseNotFoundError(ObjectDoesNotExist):
-    """ Course was not found. """
+    """
+    Course was not found.
+    """
     pass
 
 
 class PageNotFoundError(ObjectDoesNotExist):
-    """ Page was not found. Used for paginated endpoint. """
+    """
+    Page was not found. Used for paginated endpoint.
+    """
+    pass
+
+
+class DiscussionNotFoundError(ObjectDoesNotExist):
+    """
+    Discussion Module was not found.
+    """
     pass