Merge pull request #8487 from edx/clee/discussion-api-delete-comment-rebased

Clee/discussion api delete comment rebased

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

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)

lms/djangoapps/discussion_api/tests/test_views.py

@@ -558,6 +558,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"""
     def setUp(self):

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

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