diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 37646febac..899e781619 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/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) - 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) - 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()) - ] + 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) - 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]) - ) - ] + 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) + + 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, diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index c88e9941e0..e5a4097550 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/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 diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index de2bfa7644..e3489b32b0 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/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 diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 9b87c91fb8..2d1ed2a9e8 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/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 diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index f082211bd0..a34598cd11 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/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) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 16e3d3baab..801842a9f7 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/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) diff --git a/openedx/core/lib/exceptions.py b/openedx/core/lib/exceptions.py index d6e5714ac7..a140df9911 100644 --- a/openedx/core/lib/exceptions.py +++ b/openedx/core/lib/exceptions.py @@ -1,12 +1,25 @@ -""" 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