Use parent_id returned from the comments service

This depends on cs_comments_service commit 0487891.

lms/djangoapps/discussion_api/api.py

@@ -41,7 +41,7 @@ def _get_course_or_404(course_key, user):
     return course
 
 
-def _get_thread_and_context(request, thread_id, parent_id=None, retrieve_kwargs=None):
+def _get_thread_and_context(request, thread_id, retrieve_kwargs=None):
     """
     Retrieve the given thread and build a serializer context for it, returning
     both. This function also enforces access control for the thread (checking
@@ -56,7 +56,7 @@ def _get_thread_and_context(request, thread_id, parent_id=None, retrieve_kwargs=
         cc_thread = Thread(id=thread_id).retrieve(**retrieve_kwargs)
         course_key = CourseKey.from_string(cc_thread["course_id"])
         course = _get_course_or_404(course_key, request.user)
-        context = get_context(course, request, cc_thread, parent_id)
+        context = get_context(course, request, cc_thread)
         if (
                 not context["is_requester_privileged"] and
                 cc_thread["group_id"] and
@@ -350,11 +350,10 @@ def create_comment(request, comment_data):
         detail.
     """
     thread_id = comment_data.get("thread_id")
-    parent_id = comment_data.get("parent_id")
     if not thread_id:
         raise ValidationError({"thread_id": ["This field is required."]})
     try:
-        cc_thread, context = _get_thread_and_context(request, thread_id, parent_id)
+        cc_thread, context = _get_thread_and_context(request, thread_id)
     except Http404:
         raise ValidationError({"thread_id": ["Invalid value."]})
 

lms/djangoapps/discussion_api/serializers.py

@@ -24,7 +24,7 @@ from openedx.core.djangoapps.course_groups.cohorts import get_cohort_names
 from openedx.core.lib.api.fields import NonEmptyCharField
 
 
-def get_context(course, request, thread=None, parent_id=None):
+def get_context(course, request, thread=None):
     """
     Returns a context appropriate for use with ThreadSerializer or
     (if thread is provided) CommentSerializer.
@@ -48,7 +48,6 @@ def get_context(course, request, thread=None, parent_id=None):
         "course": course,
         "request": request,
         "thread": thread,
-        "parent_id": parent_id,
         # For now, the only groups are cohorts
         "group_ids_to_names": get_cohort_names(course),
         "is_requester_privileged": requester.id in staff_user_ids or requester.id in ta_user_ids,
@@ -219,25 +218,18 @@ class CommentSerializer(_ContentSerializer):
     """
     A serializer for comment data.
 
-    Because it is not a field in the underlying data, parent_id must be provided
-    in the context for both serialization and deserialization.
-
     N.B. This should not be used with a comment_client Comment object that has
     not had retrieve() called, because of the interaction between DRF's attempts
     at introspection and Comment's __getattr__.
     """
     thread_id = serializers.CharField()
-    parent_id = serializers.SerializerMethodField("get_parent_id")
+    parent_id = serializers.CharField(required=False)
     endorsed = serializers.BooleanField(read_only=True)
     endorsed_by = serializers.SerializerMethodField("get_endorsed_by")
     endorsed_by_label = serializers.SerializerMethodField("get_endorsed_by_label")
     endorsed_at = serializers.SerializerMethodField("get_endorsed_at")
     children = serializers.SerializerMethodField("get_children")
 
-    def get_parent_id(self, _obj):
-        """Returns the comment's parent's id (taken from the context)."""
-        return self.context["parent_id"]
-
     def get_endorsed_by(self, obj):
         """
         Returns the username of the endorsing user, if the information is
@@ -272,11 +264,8 @@ class CommentSerializer(_ContentSerializer):
         return endorsement["time"] if endorsement else None
 
     def get_children(self, obj):
-        """Returns the list of the comment's children, serialized."""
-        child_context = dict(self.context)
-        child_context["parent_id"] = obj["id"]
         return [
-            CommentSerializer(child, context=child_context).data
+            CommentSerializer(child, context=self.context).data
             for child in obj.get("children", [])
         ]
 
@@ -285,7 +274,7 @@ class CommentSerializer(_ContentSerializer):
         Ensure that parent_id identifies a comment that is actually in the
         thread identified by thread_id.
         """
-        parent_id = self.context["parent_id"]
+        parent_id = attrs.get("parent_id")
         if parent_id:
             parent = None
             try:
@@ -303,7 +292,6 @@ class CommentSerializer(_ContentSerializer):
             raise ValueError("CommentSerializer cannot be used for updates.")
         return Comment(
             course_id=self.context["thread"]["course_id"],
-            parent_id=self.context["parent_id"],
             user_id=self.context["cc_requester"]["id"],
             **attrs
         )

lms/djangoapps/discussion_api/tests/test_api.py

@@ -1220,12 +1220,11 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
         self.register_post_comment_response(
             {
                 "id": "test_comment",
-                "thread_id": "test_thread",
                 "username": self.user.username,
                 "created_at": "2015-05-27T00:00:00Z",
                 "updated_at": "2015-05-27T00:00:00Z",
             },
-            thread_id=(None if parent_id else "test_thread"),
+            thread_id="test_thread",
             parent_id=parent_id
         )
         data = self.minimal_data.copy()

lms/djangoapps/discussion_api/tests/test_serializers.py

@@ -354,10 +354,17 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase):
             "children": [
                 self.make_cs_content({
                     "id": "test_child_1",
+                    "parent_id": "test_root",
                 }),
                 self.make_cs_content({
                     "id": "test_child_2",
-                    "children": [self.make_cs_content({"id": "test_grandchild"})],
+                    "parent_id": "test_root",
+                    "children": [
+                        self.make_cs_content({
+                            "id": "test_grandchild",
+                            "parent_id": "test_child_2"
+                        })
+                    ],
                 }),
             ],
         })
@@ -547,7 +554,7 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore
             "raw_body": "Test body",
         }
 
-    def save_and_reserialize(self, data, parent_id=None):
+    def save_and_reserialize(self, data):
         """
         Create a serializer with the given data, ensure that it is valid, save
         the result, and return the full comment data from the serializer.
@@ -555,8 +562,7 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore
         context = get_context(
             self.course,
             self.request,
-            make_minimal_cs_thread({"course_id": unicode(self.course.id)}),
+            make_minimal_cs_thread({"course_id": unicode(self.course.id)})
-            parent_id
         )
         serializer = CommentSerializer(data=data, context=context)
         self.assertTrue(serializer.is_valid())
@@ -565,14 +571,16 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore
 
     @ddt.data(None, "test_parent")
     def test_success(self, parent_id):
+        data = self.minimal_data.copy()
         if parent_id:
+            data["parent_id"] = parent_id
             self.register_get_comment_response({"thread_id": "test_thread", "id": parent_id})
         self.register_post_comment_response(
             {"id": "test_comment"},
-            thread_id=(None if parent_id else "test_thread"),
+            thread_id="test_thread",
             parent_id=parent_id
         )
-        saved = self.save_and_reserialize(self.minimal_data, parent_id=parent_id)
+        saved = self.save_and_reserialize(data)
         expected_url = (
             "/api/v1/comments/{}".format(parent_id) if parent_id else
             "/api/v1/threads/test_thread/comments"
@@ -591,8 +599,10 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore
 
     def test_parent_id_nonexistent(self):
         self.register_get_comment_error_response("bad_parent", 404)
-        context = get_context(self.course, self.request, make_minimal_cs_thread(), "bad_parent")
+        data = self.minimal_data.copy()
-        serializer = CommentSerializer(data=self.minimal_data, context=context)
+        data["parent_id"] = "bad_parent"
+        context = get_context(self.course, self.request, make_minimal_cs_thread())
+        serializer = CommentSerializer(data=data, context=context)
         self.assertFalse(serializer.is_valid())
         self.assertEqual(
             serializer.errors,
@@ -605,8 +615,10 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore
 
     def test_parent_id_wrong_thread(self):
         self.register_get_comment_response({"thread_id": "different_thread", "id": "test_parent"})
-        context = get_context(self.course, self.request, make_minimal_cs_thread(), "test_parent")
+        data = self.minimal_data.copy()
-        serializer = CommentSerializer(data=self.minimal_data, context=context)
+        data["parent_id"] = "test_parent"
+        context = get_context(self.course, self.request, make_minimal_cs_thread())
+        serializer = CommentSerializer(data=data, context=context)
         self.assertFalse(serializer.is_valid())
         self.assertEqual(
             serializer.errors,

lms/djangoapps/discussion_api/tests/test_views.py

@@ -539,7 +539,6 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
         self.register_post_comment_response(
             {
                 "id": "test_comment",
-                "thread_id": "test_thread",
                 "username": self.user.username,
                 "created_at": "2015-05-27T00:00:00Z",
                 "updated_at": "2015-05-27T00:00:00Z",

lms/djangoapps/discussion_api/tests/utils.py

@@ -84,7 +84,7 @@ class CommentsServiceMockMixin(object):
             status=200
         )
 
-    def register_post_comment_response(self, response_overrides, thread_id=None, parent_id=None):
+    def register_post_comment_response(self, response_overrides, thread_id, parent_id=None):
         """
         Register a mock response for POST on the CS comments endpoint for the
         given thread or parent; exactly one of thread_id and parent_id must be
@@ -99,14 +99,16 @@ class CommentsServiceMockMixin(object):
                 {key: val[0] for key, val in request.parsed_body.items()}
             )
             response_data.update(response_overrides or {})
+            # thread_id and parent_id are not included in request payload but
+            # are returned by the comments service
+            response_data["thread_id"] = thread_id
+            response_data["parent_id"] = parent_id
             return (200, headers, json.dumps(response_data))
 
-        if thread_id and not parent_id:
+        if parent_id:
-            url = "http://localhost:4567/api/v1/threads/{}/comments".format(thread_id)
-        elif parent_id and not thread_id:
             url = "http://localhost:4567/api/v1/comments/{}".format(parent_id)
-        else:  # pragma: no cover
+        else:
-            raise ValueError("Exactly one of thread_id and parent_id must be provided.")
+            url = "http://localhost:4567/api/v1/threads/{}/comments".format(thread_id)
 
         httpretty.register_uri(httpretty.POST, url, body=callback)
 
@@ -228,6 +230,7 @@ def make_minimal_cs_comment(overrides=None):
     ret = {
         "id": "dummy",
         "thread_id": "dummy",
+        "parent_id": None,
         "user_id": "0",
         "username": "dummy",
         "anonymous": False,