Merge pull request #8543 from edx/gprice/discussion-api-comment-vote

Add comment voting to discussion API

lms/djangoapps/discussion_api/api.py

@@ -15,7 +15,7 @@ from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locator import CourseKey
 
 from courseware.courses import get_course_with_access
-from discussion_api.forms import ThreadActionsForm
+from discussion_api.forms import CommentActionsForm, ThreadActionsForm
 from discussion_api.pagination import get_paginated_data
 from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context
 from django_comment_client.base.views import (
@@ -352,25 +352,25 @@ def get_comment_list(request, thread_id, endorsed, page, page_size):
     return get_paginated_data(request, results, page, num_pages)
 
 
-def _do_extra_thread_actions(api_thread, cc_thread, request_fields, actions_form, context):
+def _do_extra_actions(api_content, cc_content, request_fields, actions_form, context):
     """
-    Perform any necessary additional actions related to thread creation or
+    Perform any necessary additional actions related to content creation or
     update that require a separate comments service request.
     """
     for field, form_value in actions_form.cleaned_data.items():
-        if field in request_fields and form_value != api_thread[field]:
-            api_thread[field] = form_value
+        if field in request_fields and form_value != api_content[field]:
+            api_content[field] = form_value
             if field == "following":
                 if form_value:
-                    context["cc_requester"].follow(cc_thread)
+                    context["cc_requester"].follow(cc_content)
                 else:
-                    context["cc_requester"].unfollow(cc_thread)
+                    context["cc_requester"].unfollow(cc_content)
             else:
                 assert field == "voted"
                 if form_value:
-                    context["cc_requester"].vote(cc_thread, "up")
+                    context["cc_requester"].vote(cc_content, "up")
                 else:
-                    context["cc_requester"].unvote(cc_thread)
+                    context["cc_requester"].unvote(cc_content)
 
 
 def create_thread(request, thread_data):
@@ -407,7 +407,7 @@ def create_thread(request, thread_data):
 
     cc_thread = serializer.object
     api_thread = serializer.data
-    _do_extra_thread_actions(api_thread, cc_thread, thread_data.keys(), actions_form, context)
+    _do_extra_actions(api_thread, cc_thread, thread_data.keys(), actions_form, context)
 
     track_forum_event(
         request,
@@ -445,11 +445,15 @@ def create_comment(request, comment_data):
         raise ValidationError({"thread_id": ["Invalid value."]})
 
     serializer = CommentSerializer(data=comment_data, context=context)
-    if not serializer.is_valid():
-        raise ValidationError(serializer.errors)
+    actions_form = CommentActionsForm(comment_data)
+    if not (serializer.is_valid() and actions_form.is_valid()):
+        raise ValidationError(dict(serializer.errors.items() + actions_form.errors.items()))
     serializer.save()
 
     cc_comment = serializer.object
+    api_comment = serializer.data
+    _do_extra_actions(api_comment, cc_comment, comment_data.keys(), actions_form, context)
+
     track_forum_event(
         request,
         get_comment_created_event_name(cc_comment),
@@ -518,7 +522,7 @@ def update_thread(request, thread_id, update_data):
     if set(update_data) - set(actions_form.fields):
         serializer.save()
     api_thread = serializer.data
-    _do_extra_thread_actions(api_thread, cc_thread, update_data.keys(), actions_form, context)
+    _do_extra_actions(api_thread, cc_thread, update_data.keys(), actions_form, context)
     return api_thread
 
 
@@ -530,7 +534,7 @@ def _get_comment_editable_fields(cc_comment, context):
     """
     Get the list of editable fields for the given comment in the given context
     """
-    ret = set()
+    ret = {"voted"}
     if _is_user_author_or_privileged(cc_comment, context):
         ret |= _COMMENT_EDITABLE_BY_AUTHOR
     if _is_user_author_or_privileged(context["thread"], context):
@@ -571,12 +575,15 @@ def update_comment(request, comment_id, update_data):
     editable_fields = _get_comment_editable_fields(cc_comment, context)
     _check_editable_fields(editable_fields, update_data)
     serializer = CommentSerializer(cc_comment, data=update_data, partial=True, context=context)
-    if not serializer.is_valid():
-        raise ValidationError(serializer.errors)
-    # Only save comment object if the comment is actually modified
-    if update_data:
+    actions_form = CommentActionsForm(update_data)
+    if not (serializer.is_valid() and actions_form.is_valid()):
+        raise ValidationError(dict(serializer.errors.items() + actions_form.errors.items()))
+    # Only save thread object if some of the edited fields are in the thread data, not extra actions
+    if set(update_data) - set(actions_form.fields):
         serializer.save()
-    return serializer.data
+    api_comment = serializer.data
+    _do_extra_actions(api_comment, cc_comment, update_data.keys(), actions_form, context)
+    return api_comment
 
 
 def delete_thread(request, thread_id):

lms/djangoapps/discussion_api/forms.py

@@ -75,7 +75,7 @@ class ThreadListGetForm(_PaginationForm):
 
 class ThreadActionsForm(Form):
     """
-    A form to handle fields in thread creation that require separate
+    A form to handle fields in thread creation/update that require separate
     interactions with the comments service.
     """
     following = BooleanField(required=False)
@@ -90,3 +90,11 @@ class CommentListGetForm(_PaginationForm):
     # TODO: should we use something better here? This only accepts "True",
     # "False", "1", and "0"
     endorsed = NullBooleanField(required=False)
+
+
+class CommentActionsForm(Form):
+    """
+    A form to handle fields in comment creation/update that require separate
+    interactions with the comments service.
+    """
+    voted = BooleanField(required=False)

lms/djangoapps/discussion_api/tests/test_api.py

@@ -1400,6 +1400,21 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
         self.assertEqual(actual_event_name, expected_event_name)
         self.assertEqual(actual_event_data, expected_event_data)
 
+    def test_voted(self):
+        self.register_post_comment_response({"id": "test_comment"}, "test_thread")
+        self.register_comment_votes_response("test_comment")
+        data = self.minimal_data.copy()
+        data["voted"] = "True"
+        result = create_comment(self.request, data)
+        self.assertEqual(result["voted"], True)
+        cs_request = httpretty.last_request()
+        self.assertEqual(urlparse(cs_request.path).path, "/api/v1/comments/test_comment/votes")
+        self.assertEqual(cs_request.method, "PUT")
+        self.assertEqual(
+            cs_request.parsed_body,
+            {"user_id": [str(self.user.id)], "value": ["up"]}
+        )
+
     def test_thread_id_missing(self):
         with self.assertRaises(ValidationError) as assertion:
             create_comment(self.request, {})
@@ -1959,6 +1974,45 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
                 {"endorsed": ["This field is not editable."]}
             )
 
+    @ddt.data(*itertools.product([True, False], [True, False]))
+    @ddt.unpack
+    def test_voted(self, old_voted, new_voted):
+        """
+        Test attempts to edit the "voted" field.
+
+        old_voted indicates whether the comment should be upvoted at the start of
+        the test. new_voted indicates the value for the "voted" field in the
+        update. If old_voted and new_voted are the same, no update should be
+        made. Otherwise, a vote should be PUT or DELETEd according to the
+        new_voted value.
+        """
+        if old_voted:
+            self.register_get_user_response(self.user, upvoted_ids=["test_comment"])
+        self.register_comment_votes_response("test_comment")
+        self.register_comment()
+        data = {"voted": new_voted}
+        result = update_comment(self.request, "test_comment", data)
+        self.assertEqual(result["voted"], new_voted)
+        last_request_path = urlparse(httpretty.last_request().path).path
+        votes_url = "/api/v1/comments/test_comment/votes"
+        if old_voted == new_voted:
+            self.assertNotEqual(last_request_path, votes_url)
+        else:
+            self.assertEqual(last_request_path, votes_url)
+            self.assertEqual(
+                httpretty.last_request().method,
+                "PUT" if new_voted else "DELETE"
+            )
+            actual_request_data = (
+                httpretty.last_request().parsed_body if new_voted else
+                parse_qs(urlparse(httpretty.last_request().path).query)
+            )
+            actual_request_data.pop("request_id", None)
+            expected_request_data = {"user_id": [str(self.user.id)]}
+            if new_voted:
+                expected_request_data["value"] = ["up"]
+            self.assertEqual(actual_request_data, expected_request_data)
+
 
 @ddt.ddt
 class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestCase):

lms/djangoapps/discussion_api/tests/utils.py

@@ -217,6 +217,19 @@ class CommentsServiceMockMixin(object):
                 status=200
             )
 
+    def register_comment_votes_response(self, comment_id):
+        """
+        Register a mock response for PUT and DELETE on the CS comment votes
+        endpoint
+        """
+        for method in [httpretty.PUT, httpretty.DELETE]:
+            httpretty.register_uri(
+                method,
+                "http://localhost:4567/api/v1/comments/{}/votes".format(comment_id),
+                body=json.dumps({}),  # body is unused
+                status=200
+            )
+
     def register_delete_thread_response(self, thread_id):
         """
         Register a mock response for DELETE on the CS thread instance endpoint