From 2c7590d1971d3a4d948de764908ca6c3e44b323f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 8 Jun 2015 16:59:21 -0400 Subject: [PATCH] Use parent_id returned from the comments service This depends on cs_comments_service commit 0487891. --- lms/djangoapps/discussion_api/api.py | 7 ++-- lms/djangoapps/discussion_api/serializers.py | 20 +++--------- .../discussion_api/tests/test_api.py | 3 +- .../discussion_api/tests/test_serializers.py | 32 +++++++++++++------ .../discussion_api/tests/test_views.py | 1 - lms/djangoapps/discussion_api/tests/utils.py | 15 +++++---- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index c59e88b715..041b50250f 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/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."]}) diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index 1dc137565c..81cd98bc5d 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/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 ) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 299b2e5059..7109620c75 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/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() diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py index b160658060..6607908809 100644 --- a/lms/djangoapps/discussion_api/tests/test_serializers.py +++ b/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)}), - parent_id + make_minimal_cs_thread({"course_id": unicode(self.course.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") - serializer = CommentSerializer(data=self.minimal_data, context=context) + data = self.minimal_data.copy() + 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") - serializer = CommentSerializer(data=self.minimal_data, context=context) + data = self.minimal_data.copy() + 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, diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index c5a16ee39d..502e9bb4ea 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/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", diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index add247dbd2..2e7337519e 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/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: - url = "http://localhost:4567/api/v1/threads/{}/comments".format(thread_id) - elif parent_id and not thread_id: + if parent_id: url = "http://localhost:4567/api/v1/comments/{}".format(parent_id) - else: # pragma: no cover - raise ValueError("Exactly one of thread_id and parent_id must be provided.") + else: + 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,