diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index c59e88b715..bc217c639c 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -9,6 +9,8 @@ from django.core.exceptions import ValidationError from django.core.urlresolvers import reverse from django.http import Http404 +from rest_framework.exceptions import PermissionDenied + from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey @@ -72,6 +74,19 @@ def _get_thread_and_context(request, thread_id, parent_id=None, retrieve_kwargs= raise Http404 +def _is_user_author_or_privileged(cc_thread, context): + """ + Check if the user is the author of a thread or a privileged user. + + Returns: + Boolean + """ + return ( + context["is_requester_privileged"] or + context["cc_requester"]["id"] == cc_thread["user_id"] + ) + + def get_thread_list_url(request, course_key, topic_id_list): """ Returns the URL for the thread_list_url field, given a list of topic_ids @@ -383,8 +398,7 @@ def _get_thread_editable_fields(cc_thread, context): """ Get the list of editable fields for the given thread in the given context """ - is_author = context["cc_requester"]["id"] == cc_thread["user_id"] - if context["is_requester_privileged"] or is_author: + if _is_user_author_or_privileged(cc_thread, context): return _THREAD_EDITABLE_BY_AUTHOR else: return _THREAD_EDITABLE_BY_ANY @@ -427,3 +441,26 @@ def update_thread(request, thread_id, update_data): api_thread = serializer.data _do_extra_thread_actions(api_thread, cc_thread, update_data.keys(), actions_form, context) return api_thread + + +def delete_thread(request, thread_id): + """ + Delete a thread. + + Parameters: + + request: The django request object used for build_absolute_uri and + determining the requesting user. + + thread_id: The id for the thread to delete + + Raises: + + PermissionDenied: if user does not have permission to delete thread + + """ + cc_thread, context = _get_thread_and_context(request, thread_id) + if _is_user_author_or_privileged(cc_thread, context): + cc_thread.delete() + else: + raise PermissionDenied diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 299b2e5059..bcecdf2a97 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -15,12 +15,15 @@ from django.core.exceptions import ValidationError from django.http import Http404 from django.test.client import RequestFactory +from rest_framework.exceptions import PermissionDenied + from opaque_keys.edx.locator import CourseLocator from courseware.tests.factories import BetaTesterFactory, StaffFactory from discussion_api.api import ( create_comment, create_thread, + delete_thread, get_comment_list, get_course_topics, get_thread_list, @@ -1474,7 +1477,7 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC with self.assertRaises(Http404): update_thread(self.request, "test_thread", {}) - def test_unenrolled(self): + def test_not_enrolled(self): self.register_thread() self.request.user = UserFactory.create() with self.assertRaises(Http404): @@ -1633,3 +1636,130 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC assertion.exception.message_dict, {"raw_body": ["This field is required."]} ) + + +@ddt.ddt +class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestCase): + """Tests for delete_thread""" + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(DeleteThreadTest, self).setUp() + httpretty.reset() + httpretty.enable() + self.addCleanup(httpretty.disable) + self.user = UserFactory.create() + self.register_get_user_response(self.user) + self.request = RequestFactory().get("/test_path") + self.request.user = self.user + self.course = CourseFactory.create() + self.thread_id = "test_thread" + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + + def register_thread(self, overrides=None): + """ + Make a thread with appropriate data overridden by the overrides + parameter and register mock responses for both GET and DELETE on its + endpoint. + """ + cs_data = make_minimal_cs_thread({ + "id": self.thread_id, + "course_id": unicode(self.course.id), + "user_id": str(self.user.id), + }) + cs_data.update(overrides or {}) + self.register_get_thread_response(cs_data) + self.register_delete_thread_response(cs_data["id"]) + + def test_basic(self): + self.register_thread() + self.assertIsNone(delete_thread(self.request, self.thread_id)) + self.assertEqual( + urlparse(httpretty.last_request().path).path, + "/api/v1/threads/{}".format(self.thread_id) + ) + self.assertEqual(httpretty.last_request().method, "DELETE") + + def test_thread_id_not_found(self): + self.register_get_thread_error_response("missing_thread", 404) + with self.assertRaises(Http404): + delete_thread(self.request, "missing_thread") + + def test_nonexistent_course(self): + self.register_thread({"course_id": "non/existent/course"}) + with self.assertRaises(Http404): + delete_thread(self.request, self.thread_id) + + def test_not_enrolled(self): + self.register_thread() + self.request.user = UserFactory.create() + with self.assertRaises(Http404): + delete_thread(self.request, self.thread_id) + + def test_discussions_disabled(self): + self.register_thread() + _remove_discussion_tab(self.course, self.user.id) + with self.assertRaises(Http404): + delete_thread(self.request, self.thread_id) + + @ddt.data( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ) + def test_non_author_delete_allowed(self, role_name): + role = Role.objects.create(name=role_name, course_id=self.course.id) + role.users = [self.user] + self.register_thread({"user_id": str(self.user.id + 1)}) + expected_error = role_name == FORUM_ROLE_STUDENT + try: + delete_thread(self.request, "test_thread") + self.assertFalse(expected_error) + except PermissionDenied: + self.assertTrue(expected_error) + + @ddt.data( + *itertools.product( + [ + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ], + [True, False], + ["no_group", "match_group", "different_group"], + ) + ) + @ddt.unpack + def test_group_access(self, role_name, course_is_cohorted, thread_group_state): + """ + Tests group access for deleting a thread + + All privileged roles are able to delete a thread. A student role can + only delete a thread if, + the student role is the author and the thread is not in a cohort, + the student role is the author and the thread is in the author's cohort. + """ + cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted}) + CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id) + cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user]) + role = Role.objects.create(name=role_name, course_id=cohort_course.id) + role.users = [self.user] + self.register_thread({ + "course_id": unicode(cohort_course.id), + "group_id": ( + None if thread_group_state == "no_group" else + cohort.id if thread_group_state == "match_group" else + cohort.id + 1 + ), + }) + expected_error = ( + role_name == FORUM_ROLE_STUDENT and + course_is_cohorted and + thread_group_state == "different_group" + ) + try: + delete_thread(self.request, "test_thread") + self.assertFalse(expected_error) + except Http404: + self.assertTrue(expected_error) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index c5a16ee39d..02f9b5a810 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -393,6 +393,39 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest self.assertEqual(response_data, expected_response_data) +@httpretty.activate +class ThreadViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): + """Tests for ThreadViewSet delete""" + def setUp(self): + super(ThreadViewSetDeleteTest, self).setUp() + self.url = reverse("thread-detail", kwargs={"thread_id": "test_thread"}) + self.thread_id = "test_thread" + + def test_basic(self): + self.register_get_user_response(self.user) + cs_thread = make_minimal_cs_thread({ + "id": self.thread_id, + "course_id": unicode(self.course.id), + "username": self.user.username, + "user_id": str(self.user.id), + }) + self.register_get_thread_response(cs_thread) + self.register_delete_thread_response(self.thread_id) + response = self.client.delete(self.url) + self.assertEqual(response.status_code, 204) + self.assertEqual(response.content, "") + self.assertEqual( + urlparse(httpretty.last_request().path).path, + "/api/v1/threads/{}".format(self.thread_id) + ) + self.assertEqual(httpretty.last_request().method, "DELETE") + + def test_delete_nonexistent_thread(self): + self.register_get_thread_error_response(self.thread_id, 404) + response = self.client.delete(self.url) + self.assertEqual(response.status_code, 404) + + @httpretty.activate class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CommentViewSet list""" diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index add247dbd2..7b278443e6 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -173,6 +173,17 @@ class CommentsServiceMockMixin(object): status=200 ) + def register_delete_thread_response(self, thread_id): + """ + Register a mock response for DELETE on the CS thread instance endpoint + """ + httpretty.register_uri( + httpretty.DELETE, + "http://localhost:4567/api/v1/threads/{id}".format(id=thread_id), + body=json.dumps({}), # body is unused + status=200 + ) + def assert_query_params_equal(self, httpretty_request, expected_params): """ Assert that the given mock request had the expected query parameters diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index d2cc19f6b7..dae2569d10 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -14,6 +14,7 @@ from opaque_keys.edx.locator import CourseLocator from discussion_api.api import ( create_comment, create_thread, + delete_thread, get_comment_list, get_course_topics, get_thread_list, @@ -69,7 +70,7 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): **Use Cases** Retrieve the list of threads for a course, post a new thread, or modify - an existing thread. + or delete an existing thread. **Example Requests**: @@ -87,6 +88,8 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): PATCH /api/discussion/v1/threads/thread_id {"raw_body": "Edited text"} + DELETE /api/discussion/v1/threads/thread_id + **GET Parameters**: * course_id (required): The course to retrieve threads for @@ -157,6 +160,10 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): that were created or updated since the last time the user read the thread + **DELETE response values: + + No content is returned for a DELETE request + """ lookup_field = "thread_id" @@ -192,6 +199,14 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): """ return Response(update_thread(request, thread_id, request.DATA)) + def destroy(self, request, thread_id): + """ + Implements the DELETE method for the instance endpoint as described in + the class docstring + """ + delete_thread(request, thread_id) + return Response(status=204) + class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): """