diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 4f3565a056..a14bcae278 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -525,3 +525,26 @@ def delete_thread(request, thread_id): cc_thread.delete() else: raise PermissionDenied + + +def delete_comment(request, comment_id): + """ + Delete a comment. + + Parameters: + + request: The django request object used for build_absolute_uri and + determining the requesting user. + + comment_id: The id of the comment to delete + + Raises: + + PermissionDenied: if user does not have permission to delete thread + + """ + cc_comment, context = _get_comment_and_context(request, comment_id) + if _is_user_author_or_privileged(cc_comment, context): + cc_comment.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 73354802b1..be157a4273 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -23,6 +23,7 @@ from courseware.tests.factories import BetaTesterFactory, StaffFactory from discussion_api.api import ( create_comment, create_thread, + delete_comment, delete_thread, get_comment_list, get_course_topics, @@ -1880,7 +1881,7 @@ class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC self.register_thread({"user_id": str(self.user.id + 1)}) expected_error = role_name == FORUM_ROLE_STUDENT try: - delete_thread(self.request, "test_thread") + delete_thread(self.request, self.thread_id) self.assertFalse(expected_error) except PermissionDenied: self.assertTrue(expected_error) @@ -1926,7 +1927,150 @@ class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC thread_group_state == "different_group" ) try: - delete_thread(self.request, "test_thread") + delete_thread(self.request, self.thread_id) + self.assertFalse(expected_error) + except Http404: + self.assertTrue(expected_error) + + +@ddt.ddt +class DeleteCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestCase): + """Tests for delete_comment""" + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(DeleteCommentTest, 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" + self.comment_id = "test_comment" + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + + def register_comment_and_thread(self, overrides=None, thread_overrides=None): + """ + Make a comment with appropriate data overridden by the override + parameters and register mock responses for both GET and DELETE on its + endpoint. Also mock GET for the related thread with thread_overrides. + """ + cs_thread_data = make_minimal_cs_thread({ + "id": self.thread_id, + "course_id": unicode(self.course.id) + }) + cs_thread_data.update(thread_overrides or {}) + self.register_get_thread_response(cs_thread_data) + cs_comment_data = make_minimal_cs_comment({ + "id": self.comment_id, + "course_id": cs_thread_data["course_id"], + "thread_id": cs_thread_data["id"], + "username": self.user.username, + "user_id": str(self.user.id), + }) + cs_comment_data.update(overrides or {}) + self.register_get_comment_response(cs_comment_data) + self.register_delete_comment_response(self.comment_id) + + def test_basic(self): + self.register_comment_and_thread() + self.assertIsNone(delete_comment(self.request, self.comment_id)) + self.assertEqual( + urlparse(httpretty.last_request().path).path, + "/api/v1/comments/{}".format(self.comment_id) + ) + self.assertEqual(httpretty.last_request().method, "DELETE") + + def test_comment_id_not_found(self): + self.register_get_comment_error_response("missing_comment", 404) + with self.assertRaises(Http404): + delete_comment(self.request, "missing_comment") + + def test_nonexistent_course(self): + self.register_comment_and_thread( + thread_overrides={"course_id": "non/existent/course"} + ) + with self.assertRaises(Http404): + delete_comment(self.request, self.comment_id) + + def test_not_enrolled(self): + self.register_comment_and_thread() + self.request.user = UserFactory.create() + with self.assertRaises(Http404): + delete_comment(self.request, self.comment_id) + + def test_discussions_disabled(self): + self.register_comment_and_thread() + _remove_discussion_tab(self.course, self.user.id) + with self.assertRaises(Http404): + delete_comment(self.request, self.comment_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_comment_and_thread( + overrides={"user_id": str(self.user.id + 1)} + ) + expected_error = role_name == FORUM_ROLE_STUDENT + try: + delete_comment(self.request, self.comment_id) + 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 comment + + All privileged roles are able to delete a comment. A student role can + only delete a comment if, + the student role is the author and the comment is not in a cohort, + the student role is the author and the comment 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_comment_and_thread( + overrides={"thread_id": "test_thread"}, + thread_overrides={ + "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_comment(self.request, self.comment_id) 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 cb2ad65513..1b1b038a3d 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -557,6 +557,46 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) +@httpretty.activate +class CommentViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): + """Tests for ThreadViewSet delete""" + + def setUp(self): + super(CommentViewSetDeleteTest, self).setUp() + self.url = reverse("comment-detail", kwargs={"comment_id": "test_comment"}) + self.comment_id = "test_comment" + + def test_basic(self): + self.register_get_user_response(self.user) + cs_thread = make_minimal_cs_thread({ + "id": "test_thread", + "course_id": unicode(self.course.id), + }) + self.register_get_thread_response(cs_thread) + cs_comment = make_minimal_cs_comment({ + "id": self.comment_id, + "course_id": cs_thread["course_id"], + "thread_id": cs_thread["id"], + "username": self.user.username, + "user_id": str(self.user.id), + }) + self.register_get_comment_response(cs_comment) + self.register_delete_comment_response(self.comment_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/comments/{}".format(self.comment_id) + ) + self.assertEqual(httpretty.last_request().method, "DELETE") + + def test_delete_nonexistent_comment(self): + self.register_get_comment_error_response(self.comment_id, 404) + response = self.client.delete(self.url) + self.assertEqual(response.status_code, 404) + + @httpretty.activate class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CommentViewSet create""" diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 6ac30accf1..32a4e9dcfe 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -214,6 +214,17 @@ class CommentsServiceMockMixin(object): status=200 ) + def register_delete_comment_response(self, comment_id): + """ + Register a mock response for DELETE on the CS comment instance endpoint + """ + httpretty.register_uri( + httpretty.DELETE, + "http://localhost:4567/api/v1/comments/{id}".format(id=comment_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 7d6acb667e..500ae5d570 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -15,6 +15,7 @@ from discussion_api.api import ( create_comment, create_thread, delete_thread, + delete_comment, get_comment_list, get_course_topics, get_thread_list, @@ -214,7 +215,7 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): **Use Cases** Retrieve the list of comments in a thread, create a comment, or modify - an existing comment. + or delete an existing comment. **Example Requests**: @@ -229,6 +230,8 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): PATCH /api/discussion/v1/comments/comment_id {"raw_body": "Edited text"} + DELETE /api/discussion/v1/comments/comment_id + **GET Parameters**: * thread_id (required): The thread to retrieve comments for @@ -306,6 +309,11 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): * vote_count: The number of votes for the comment * children: The list of child comments (with the same format) + + **DELETE Response Value** + + No content is returned for a DELETE request + """ lookup_field = "comment_id" @@ -334,6 +342,14 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): """ return Response(create_comment(request, request.DATA)) + def destroy(self, request, comment_id): + """ + Implements the DELETE method for the instance endpoint as described in + the class docstring + """ + delete_comment(request, comment_id) + return Response(status=204) + def partial_update(self, request, comment_id): """ Implements the PATCH method for the instance endpoint as described in