diff --git a/lms/djangoapps/discussion_api/__init__.py b/lms/djangoapps/discussion_api/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py new file mode 100644 index 0000000000..21034f5aa7 --- /dev/null +++ b/lms/djangoapps/discussion_api/api.py @@ -0,0 +1,65 @@ +""" +Discussion API internal interface +""" +from collections import defaultdict + +from django_comment_client.utils import get_accessible_discussion_modules + + +def get_course_topics(course, user): + """ + Return the course topic listing for the given course and user. + + Parameters: + + course: The course to get topics for + user: The requesting user, for access control + + Returns: + + A course topic listing dictionary; see discussion_api.views.CourseTopicViews + for more detail. + """ + 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 + + discussion_modules = get_accessible_discussion_modules(course, user) + modules_by_category = defaultdict(list) + for module in discussion_modules: + modules_by_category[module.discussion_category].append(module) + courseware_topics = [ + { + "id": None, + "name": category, + "children": [ + { + "id": module.discussion_id, + "name": module.discussion_target, + "children": [], + } + for module in sorted(modules_by_category[category], key=get_module_sort_key) + ], + } + for category in sorted(modules_by_category.keys()) + ] + + non_courseware_topics = [ + { + "id": entry["id"], + "name": name, + "children": [], + } + for name, entry in sorted( + course.discussion_topics.items(), + key=lambda item: item[1].get("sort_key", item[0]) + ) + ] + + return { + "courseware_topics": courseware_topics, + "non_courseware_topics": non_courseware_topics, + } diff --git a/lms/djangoapps/discussion_api/models.py b/lms/djangoapps/discussion_api/models.py new file mode 100644 index 0000000000..d2e8572729 --- /dev/null +++ b/lms/djangoapps/discussion_api/models.py @@ -0,0 +1,3 @@ +""" +A models.py is required to make this an app (until we move to Django 1.7) +""" diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py new file mode 100644 index 0000000000..7b034183f6 --- /dev/null +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -0,0 +1,306 @@ +""" +Tests for Discussion API internal interface +""" +from datetime import datetime, timedelta + +import mock +from pytz import UTC + +from courseware.tests.factories import BetaTesterFactory, StaffFactory +from discussion_api.api import get_course_topics +from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.partitions.partitions import Group, UserPartition + + +@mock.patch.dict("django.conf.settings.FEATURES", {"DISABLE_START_DATES": False}) +class GetCourseTopicsTest(ModuleStoreTestCase): + """Test for get_course_topics""" + + def setUp(self): + super(GetCourseTopicsTest, self).setUp() + self.maxDiff = None # pylint: disable=invalid-name + self.partition = UserPartition( + 0, + "partition", + "Test Partition", + [Group(0, "Cohort A"), Group(1, "Cohort B")], + scheme_id="cohort" + ) + self.course = CourseFactory.create( + org="x", + course="y", + run="z", + start=datetime.now(UTC), + discussion_topics={}, + user_partitions=[self.partition], + cohort_config={"cohorted": True}, + days_early_for_beta=3 + ) + self.user = UserFactory.create() + + 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 get_course_topics(self, user=None): + """ + Get course topics for self.course, using the given user or self.user if + not provided, and generating absolute URIs with a test scheme/host. + """ + return get_course_topics(self.course, user or self.user) + + def make_expected_tree(self, topic_id, name, children=None): + """ + Build an expected result tree given a topic id, display name, and + children + """ + children = children or [] + node = { + "id": topic_id, + "name": name, + "children": children, + } + return node + + def test_empty(self): + actual = self.get_course_topics() + expected = { + "courseware_topics": [], + "non_courseware_topics": [], + } + self.assertEqual(actual, expected) + + def test_non_courseware(self): + self.course.discussion_topics = {"Topic Name": {"id": "topic-id"}} + self.course.save() + actual = self.get_course_topics() + expected = { + "courseware_topics": [], + "non_courseware_topics": [self.make_expected_tree("topic-id", "Topic Name")], + } + self.assertEqual(actual, expected) + + def test_courseware(self): + self.make_discussion_module("topic-id", "Foo", "Bar") + actual = self.get_course_topics() + expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "Foo", + [self.make_expected_tree("topic-id", "Bar")] + ), + ], + "non_courseware_topics": [], + } + self.assertEqual(actual, expected) + + def test_many(self): + self.make_discussion_module("courseware-1", "A", "1") + self.make_discussion_module("courseware-2", "A", "2") + self.make_discussion_module("courseware-3", "B", "1") + self.make_discussion_module("courseware-4", "B", "2") + self.make_discussion_module("courseware-5", "C", "1") + self.course.discussion_topics = { + "A": {"id": "non-courseware-1"}, + "B": {"id": "non-courseware-2"}, + } + self.course.save() + actual = self.get_course_topics() + expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "A", + [ + self.make_expected_tree("courseware-1", "1"), + self.make_expected_tree("courseware-2", "2"), + ] + ), + self.make_expected_tree( + None, + "B", + [ + self.make_expected_tree("courseware-3", "1"), + self.make_expected_tree("courseware-4", "2"), + ] + ), + self.make_expected_tree( + None, + "C", + [self.make_expected_tree("courseware-5", "1")] + ), + ], + "non_courseware_topics": [ + self.make_expected_tree("non-courseware-1", "A"), + self.make_expected_tree("non-courseware-2", "B"), + ], + } + self.assertEqual(actual, expected) + + def test_sort_key(self): + self.make_discussion_module("courseware-1", "First", "A", sort_key="D") + self.make_discussion_module("courseware-2", "First", "B", sort_key="B") + self.make_discussion_module("courseware-3", "First", "C", sort_key="E") + self.make_discussion_module("courseware-4", "Second", "A", sort_key="F") + self.make_discussion_module("courseware-5", "Second", "B", sort_key="G") + self.make_discussion_module("courseware-6", "Second", "C") + self.make_discussion_module("courseware-7", "Second", "D", sort_key="A") + self.course.discussion_topics = { + "W": {"id": "non-courseware-1", "sort_key": "Z"}, + "X": {"id": "non-courseware-2"}, + "Y": {"id": "non-courseware-3", "sort_key": "Y"}, + "Z": {"id": "non-courseware-4", "sort_key": "W"}, + } + self.course.save() + actual = self.get_course_topics() + expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "First", + [ + self.make_expected_tree("courseware-2", "B"), + self.make_expected_tree("courseware-1", "A"), + self.make_expected_tree("courseware-3", "C"), + ] + ), + self.make_expected_tree( + None, + "Second", + [ + self.make_expected_tree("courseware-7", "D"), + self.make_expected_tree("courseware-6", "C"), + self.make_expected_tree("courseware-4", "A"), + self.make_expected_tree("courseware-5", "B"), + ] + ), + ], + "non_courseware_topics": [ + self.make_expected_tree("non-courseware-4", "Z"), + self.make_expected_tree("non-courseware-2", "X"), + self.make_expected_tree("non-courseware-3", "Y"), + self.make_expected_tree("non-courseware-1", "W"), + ], + } + self.assertEqual(actual, expected) + + def test_access_control(self): + """ + Test that only topics that a user has access to are returned. The + ways in which a user may not have access are: + + * Module is visible to staff only + * Module has a start date in the future + * Module is accessible only to a group the user is not in + + Also, there is a case that ensures that a category with no accessible + subcategories does not appear in the result. + """ + beta_tester = BetaTesterFactory.create(course_key=self.course.id) + staff = StaffFactory.create(course_key=self.course.id) + for user, group_idx in [(self.user, 0), (beta_tester, 1)]: + cohort = CohortFactory.create( + course_id=self.course.id, + name=self.partition.groups[group_idx].name, + users=[user] + ) + CourseUserGroupPartitionGroup.objects.create( + course_user_group=cohort, + partition_id=self.partition.id, + group_id=self.partition.groups[group_idx].id + ) + + self.make_discussion_module("courseware-1", "First", "Everybody") + self.make_discussion_module( + "courseware-2", + "First", + "Cohort A", + group_access={self.partition.id: [self.partition.groups[0].id]} + ) + self.make_discussion_module( + "courseware-3", + "First", + "Cohort B", + group_access={self.partition.id: [self.partition.groups[1].id]} + ) + self.make_discussion_module("courseware-4", "Second", "Staff Only", visible_to_staff_only=True) + self.make_discussion_module( + "courseware-5", + "Second", + "Future Start Date", + start=datetime.now(UTC) + timedelta(days=1) + ) + + student_actual = self.get_course_topics() + student_expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "First", + [ + self.make_expected_tree("courseware-2", "Cohort A"), + self.make_expected_tree("courseware-1", "Everybody"), + ] + ), + ], + "non_courseware_topics": [], + } + self.assertEqual(student_actual, student_expected) + + beta_actual = self.get_course_topics(beta_tester) + beta_expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "First", + [ + self.make_expected_tree("courseware-3", "Cohort B"), + self.make_expected_tree("courseware-1", "Everybody"), + ] + ), + self.make_expected_tree( + None, + "Second", + [self.make_expected_tree("courseware-5", "Future Start Date")] + ), + ], + "non_courseware_topics": [], + } + self.assertEqual(beta_actual, beta_expected) + + staff_actual = self.get_course_topics(staff) + staff_expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "First", + [ + self.make_expected_tree("courseware-2", "Cohort A"), + self.make_expected_tree("courseware-3", "Cohort B"), + self.make_expected_tree("courseware-1", "Everybody"), + ] + ), + self.make_expected_tree( + None, + "Second", + [ + self.make_expected_tree("courseware-5", "Future Start Date"), + self.make_expected_tree("courseware-4", "Staff Only"), + ] + ), + ], + "non_courseware_topics": [], + } + self.assertEqual(staff_actual, staff_expected) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py new file mode 100644 index 0000000000..c6d213963e --- /dev/null +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -0,0 +1,100 @@ +""" +Tests for Discussion API views +""" +from datetime import datetime +import json + +import mock +from pytz import UTC + +from django.core.urlresolvers import reverse + +from student.tests.factories import CourseEnrollmentFactory, UserFactory +from util.testing import UrlResetMixin +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.tabs import DiscussionTab + + +class CourseTopicsViewTest(UrlResetMixin, ModuleStoreTestCase): + """Tests for CourseTopicsView""" + + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(CourseTopicsViewTest, self).setUp() + self.maxDiff = None # pylint: disable=invalid-name + self.course = CourseFactory.create( + org="x", + course="y", + run="z", + start=datetime.now(UTC), + discussion_topics={"Test Topic": {"id": "test_topic"}} + ) + self.password = "password" + self.user = UserFactory.create(password=self.password) + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + self.url = reverse("course_topics", kwargs={"course_id": unicode(self.course.id)}) + self.client.login(username=self.user.username, password=self.password) + + def assert_response_correct(self, response, expected_status, expected_content): + """ + Assert that the response has the given status code and parsed content + """ + self.assertEqual(response.status_code, expected_status) + parsed_content = json.loads(response.content) + self.assertEqual(parsed_content, expected_content) + + def test_not_authenticated(self): + self.client.logout() + response = self.client.get(self.url) + self.assert_response_correct( + response, + 401, + {"developer_message": "Authentication credentials were not provided."} + ) + + def test_non_existent_course(self): + response = self.client.get( + reverse("course_topics", kwargs={"course_id": "non/existent/course"}) + ) + self.assert_response_correct( + response, + 404, + {"developer_message": "Not found."} + ) + + def test_not_enrolled(self): + unenrolled_user = UserFactory.create(password=self.password) + self.client.login(username=unenrolled_user.username, password=self.password) + response = self.client.get(self.url) + self.assert_response_correct( + response, + 404, + {"developer_message": "Not found."} + ) + + def test_discussions_disabled(self): + self.course.tabs = [tab for tab in self.course.tabs if not isinstance(tab, DiscussionTab)] + modulestore().update_item(self.course, self.user.id) + response = self.client.get(self.url) + self.assert_response_correct( + response, + 404, + {"developer_message": "Not found."} + ) + + def test_get(self): + response = self.client.get(self.url) + self.assert_response_correct( + response, + 200, + { + "courseware_topics": [], + "non_courseware_topics": [{ + "id": "test_topic", + "name": "Test Topic", + "children": [] + }], + } + ) diff --git a/lms/djangoapps/discussion_api/urls.py b/lms/djangoapps/discussion_api/urls.py new file mode 100644 index 0000000000..2f023eda57 --- /dev/null +++ b/lms/djangoapps/discussion_api/urls.py @@ -0,0 +1,17 @@ +""" +Discussion API URLs +""" +from django.conf import settings +from django.conf.urls import patterns, url + +from discussion_api.views import CourseTopicsView + + +urlpatterns = patterns( + "discussion_api", + url( + r"^v1/course_topics/{}".format(settings.COURSE_ID_PATTERN), + CourseTopicsView.as_view(), + name="course_topics" + ), +) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py new file mode 100644 index 0000000000..f9d82f8b58 --- /dev/null +++ b/lms/djangoapps/discussion_api/views.py @@ -0,0 +1,54 @@ +""" +Discussion API views +""" +from django.http import Http404 + +from rest_framework.authentication import OAuth2Authentication, SessionAuthentication +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response +from rest_framework.views import APIView + +from opaque_keys.edx.locator import CourseLocator + +from courseware.courses import get_course_with_access +from discussion_api.api import get_course_topics +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin +from xmodule.tabs import DiscussionTab + + +class CourseTopicsView(DeveloperErrorViewMixin, APIView): + """ + **Use Cases** + + Retrieve the topic listing for a course. Only topics accessible to the + authenticated user are included. + + **Example Requests**: + + GET /api/discussion/v1/course_topics/{course_id} + + **Response Values**: + + * courseware_topics: The list of topic trees for courseware-linked + 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). + + * name: The display name of the topic. + + * children: A list of child subtrees of the same format. + + * non_courseware_topics: The list of topic trees that are not linked to + courseware. Items are of the same format as in courseware_topics. + """ + authentication_classes = (OAuth2Authentication, SessionAuthentication) + permission_classes = (IsAuthenticated,) + + def get(self, request, course_id): + """Implements the GET method as described in the class docstring.""" + course_key = CourseLocator.from_string(course_id) + course = get_course_with_access(request.user, 'load_forum', course_key) + if not any([isinstance(tab, DiscussionTab) for tab in course.tabs]): + raise Http404 + return Response(get_course_topics(course, request.user)) diff --git a/lms/envs/common.py b/lms/envs/common.py index ac4e49cb7a..99add8d9b2 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1657,6 +1657,7 @@ INSTALLED_APPS = ( # Discussion forums 'django_comment_client', 'django_comment_common', + 'discussion_api', 'notes', 'edxnotes', diff --git a/lms/urls.py b/lms/urls.py index c359412e97..d1baa5e6b3 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -443,6 +443,7 @@ if settings.COURSEWARE_ENABLED: # discussion forums live within courseware, so courseware must be enabled first if settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE'): urlpatterns += ( + url(r'^api/discussion/', include('discussion_api.urls')), url(r'^courses/{}/discussion/'.format(settings.COURSE_ID_PATTERN), include('django_comment_client.urls')), url(r'^notification_prefs/enable/', 'notification_prefs.views.ajax_enable'), diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py new file mode 100644 index 0000000000..9ab8c57135 --- /dev/null +++ b/openedx/core/lib/api/view_utils.py @@ -0,0 +1,28 @@ +""" +Utilities related to API views +""" +from django.http import Http404 + +from rest_framework.exceptions import APIException +from rest_framework.response import Response + + +class DeveloperErrorViewMixin(object): + """ + A view mixin to handle common error cases other than validation failure + (auth failure, method not allowed, etc.) by generating an error response + conforming to our API conventions with a developer message. + """ + def make_error_response(self, status_code, developer_message): + """ + Build an error response with the given status code and developer_message + """ + return Response({"developer_message": developer_message}, status=status_code) + + def handle_exception(self, exc): + if isinstance(exc, APIException): + return self.make_error_response(exc.status_code, exc.detail) + elif isinstance(exc, Http404): + return self.make_error_response(404, "Not found.") + else: + raise