Merge pull request #8562 from edx/gprice/discussion-api-editable-fields

Add editable_fields to discussion API responses

lms/djangoapps/discussion_api/api.py

@@ -17,6 +17,7 @@ from opaque_keys.edx.locator import CourseKey
 from courseware.courses import get_course_with_access
 from discussion_api.forms import CommentActionsForm, ThreadActionsForm
 from discussion_api.pagination import get_paginated_data
+from discussion_api.permissions import can_delete, get_editable_fields
 from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context
 from django_comment_client.base.views import (
     THREAD_CREATED_EVENT_NAME,
@@ -91,19 +92,6 @@ def _get_comment_and_context(request, comment_id):
         raise Http404
 
 
-def _is_user_author_or_privileged(cc_content, context):
-    """
-    Check if the user is the author of a content object or a privileged user.
-
-    Returns:
-        Boolean
-    """
-    return (
-        context["is_requester_privileged"] or
-        context["cc_requester"]["id"] == cc_content["user_id"]
-    )
-
-
 def get_thread_list_url(request, course_key, topic_id_list=None):
     """
     Returns the URL for the thread_list_url field, given a list of topic_ids
@@ -352,6 +340,21 @@ def get_comment_list(request, thread_id, endorsed, page, page_size):
     return get_paginated_data(request, results, page, num_pages)
 
 
+def _check_editable_fields(cc_content, data, context):
+    """
+    Raise ValidationError if the given update data contains a field that is not
+    in editable_fields.
+    """
+    editable_fields = get_editable_fields(cc_content, context)
+    non_editable_errors = {
+        field: ["This field is not editable."]
+        for field in data.keys()
+        if field not in editable_fields
+    }
+    if non_editable_errors:
+        raise ValidationError(non_editable_errors)
+
+
 def _do_extra_actions(api_content, cc_content, request_fields, actions_form, context):
     """
     Perform any necessary additional actions related to content creation or
@@ -462,35 +465,7 @@ def create_comment(request, comment_data):
         get_comment_created_event_data(cc_comment, cc_thread["commentable_id"], followed=False)
     )
 
-    return serializer.data
-
-
-_THREAD_EDITABLE_BY_ANY = {"following", "voted"}
-_THREAD_EDITABLE_BY_AUTHOR = {"topic_id", "type", "title", "raw_body"} | _THREAD_EDITABLE_BY_ANY
-
-
-def _get_thread_editable_fields(cc_thread, context):
-    """
-    Get the list of editable fields for the given thread in the given context
-    """
-    if _is_user_author_or_privileged(cc_thread, context):
-        return _THREAD_EDITABLE_BY_AUTHOR
-    else:
-        return _THREAD_EDITABLE_BY_ANY
-
-
-def _check_editable_fields(editable_fields, update_data):
-    """
-    Raise ValidationError if the given update data contains a field that is not
-    in editable_fields.
-    """
-    non_editable_errors = {
-        field: ["This field is not editable."]
-        for field in update_data.keys()
-        if field not in editable_fields
-    }
-    if non_editable_errors:
-        raise ValidationError(non_editable_errors)
+    return api_comment
 
 
 def update_thread(request, thread_id, update_data):
@@ -512,8 +487,7 @@ def update_thread(request, thread_id, update_data):
         detail.
     """
     cc_thread, context = _get_thread_and_context(request, thread_id)
-    editable_fields = _get_thread_editable_fields(cc_thread, context)
-    _check_editable_fields(editable_fields, update_data)
+    _check_editable_fields(cc_thread, update_data, context)
     serializer = ThreadSerializer(cc_thread, data=update_data, partial=True, context=context)
     actions_form = ThreadActionsForm(update_data)
     if not (serializer.is_valid() and actions_form.is_valid()):
@@ -526,22 +500,6 @@ def update_thread(request, thread_id, update_data):
     return api_thread
 
 
-_COMMENT_EDITABLE_BY_AUTHOR = {"raw_body"}
-_COMMENT_EDITABLE_BY_THREAD_AUTHOR = {"endorsed"}
-
-
-def _get_comment_editable_fields(cc_comment, context):
-    """
-    Get the list of editable fields for the given comment in the given context
-    """
-    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):
-        ret |= _COMMENT_EDITABLE_BY_THREAD_AUTHOR
-    return ret
-
-
 def update_comment(request, comment_id, update_data):
     """
     Update a comment.
@@ -572,13 +530,12 @@ def update_comment(request, comment_id, update_data):
           is empty or thread_id is included)
     """
     cc_comment, context = _get_comment_and_context(request, comment_id)
-    editable_fields = _get_comment_editable_fields(cc_comment, context)
-    _check_editable_fields(editable_fields, update_data)
+    _check_editable_fields(cc_comment, update_data, context)
     serializer = CommentSerializer(cc_comment, data=update_data, partial=True, context=context)
     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
+    # Only save comment object if some of the edited fields are in the comment data, not extra actions
     if set(update_data) - set(actions_form.fields):
         serializer.save()
     api_comment = serializer.data
@@ -603,7 +560,7 @@ def delete_thread(request, thread_id):
 
     """
     cc_thread, context = _get_thread_and_context(request, thread_id)
-    if _is_user_author_or_privileged(cc_thread, context):
+    if can_delete(cc_thread, context):
         cc_thread.delete()
     else:
         raise PermissionDenied
@@ -626,7 +583,7 @@ def delete_comment(request, comment_id):
 
     """
     cc_comment, context = _get_comment_and_context(request, comment_id)
-    if _is_user_author_or_privileged(cc_comment, context):
+    if can_delete(cc_comment, context):
         cc_comment.delete()
     else:
         raise PermissionDenied

lms/djangoapps/discussion_api/permissions.py

+"""
+Discussion API permission logic
+"""
+
+
+def _is_author(cc_content, context):
+    """
+    Return True if the requester authored the given content, False otherwise
+    """
+    return context["cc_requester"]["id"] == cc_content["user_id"]
+
+
+def _is_author_or_privileged(cc_content, context):
+    """
+    Return True if the requester authored the given content or is a privileged
+    user, False otherwise
+    """
+    return context["is_requester_privileged"] or _is_author(cc_content, context)
+
+
+def get_editable_fields(cc_content, context):
+    """
+    Return the set of fields that the requester can edit on the given content
+    """
+    # Shared fields
+    ret = {"voted"}
+    if _is_author_or_privileged(cc_content, context):
+        ret |= {"raw_body"}
+
+    # Thread fields
+    if cc_content["type"] == "thread":
+        ret |= {"following"}
+        if _is_author_or_privileged(cc_content, context):
+            ret |= {"topic_id", "type", "title"}
+
+    # Comment fields
+    if (
+            cc_content["type"] == "comment" and (
+                context["is_requester_privileged"] or (
+                    _is_author(context["thread"], context) and
+                    context["thread"]["thread_type"] == "question"
+                )
+            )
+    ):
+        ret |= {"endorsed"}
+
+    return ret
+
+
+def can_delete(cc_content, context):
+    """
+    Return True if the requester can delete the given content, False otherwise
+    """
+    return _is_author_or_privileged(cc_content, context)

lms/djangoapps/discussion_api/serializers.py

@@ -10,6 +10,7 @@ from django.core.urlresolvers import reverse
 
 from rest_framework import serializers
 
+from discussion_api.permissions import get_editable_fields
 from discussion_api.render import render_body
 from django_comment_common.models import (
     FORUM_ROLE_ADMINISTRATOR,
@@ -70,6 +71,7 @@ class _ContentSerializer(serializers.Serializer):
     abuse_flagged = serializers.SerializerMethodField("get_abuse_flagged")
     voted = serializers.SerializerMethodField("get_voted")
     vote_count = serializers.SerializerMethodField("get_vote_count")
+    editable_fields = serializers.SerializerMethodField("get_editable_fields")
 
     non_updatable_fields = ()
 
@@ -146,6 +148,10 @@ class _ContentSerializer(serializers.Serializer):
         """Returns the number of votes for the content."""
         return obj["votes"]["up_count"]
 
+    def get_editable_fields(self, obj):
+        """Return the list of the fields the requester can edit"""
+        return sorted(get_editable_fields(obj, self.context))
+
 
 class ThreadSerializer(_ContentSerializer):
     """

lms/djangoapps/discussion_api/tests/test_api.py

@@ -541,6 +541,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
     def test_thread_content(self):
         source_threads = [
             {
+                "type": "thread",
                 "id": "test_thread_id_0",
                 "course_id": unicode(self.course.id),
                 "commentable_id": "topic_x",
@@ -562,6 +563,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
                 "unread_comments_count": 3,
             },
             {
+                "type": "thread",
                 "id": "test_thread_id_1",
                 "course_id": unicode(self.course.id),
                 "commentable_id": "topic_y",
@@ -609,6 +611,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
                 "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread_id_0",
                 "endorsed_comment_list_url": None,
                 "non_endorsed_comment_list_url": None,
+                "editable_fields": ["following", "voted"],
             },
             {
                 "id": "test_thread_id_1",
@@ -639,6 +642,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
                 "non_endorsed_comment_list_url": (
                     "http://testserver/api/discussion/v1/comments/?thread_id=test_thread_id_1&endorsed=False"
                 ),
+                "editable_fields": ["following", "voted"],
             },
         ]
         self.assertEqual(
@@ -915,6 +919,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase):
     def test_discussion_content(self):
         source_comments = [
             {
+                "type": "comment",
                 "id": "test_comment_1",
                 "thread_id": "test_thread",
                 "user_id": str(self.author.id),
@@ -930,6 +935,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase):
                 "children": [],
             },
             {
+                "type": "comment",
                 "id": "test_comment_2",
                 "thread_id": "test_thread",
                 "user_id": str(self.author.id),
@@ -964,6 +970,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase):
                 "voted": False,
                 "vote_count": 4,
                 "children": [],
+                "editable_fields": ["voted"],
             },
             {
                 "id": "test_comment_2",
@@ -983,6 +990,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase):
                 "voted": False,
                 "vote_count": 7,
                 "children": [],
+                "editable_fields": ["voted"],
             },
         ]
         actual_comments = self.get_comment_list(
@@ -1202,6 +1210,7 @@ class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_id",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"],
         }
         self.assertEqual(actual, expected)
         self.assertEqual(
@@ -1367,6 +1376,7 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
             "voted": False,
             "vote_count": 0,
             "children": [],
+            "editable_fields": ["raw_body", "voted"]
         }
         self.assertEqual(actual, expected)
         expected_url = (
@@ -1535,7 +1545,7 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC
             "user_id": str(self.user.id),
             "created_at": "2015-05-29T00:00:00Z",
             "updated_at": "2015-05-29T00:00:00Z",
-            "type": "discussion",
+            "thread_type": "discussion",
             "title": "Original Title",
             "body": "Original body",
         })
@@ -1580,6 +1590,7 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"],
         }
         self.assertEqual(actual, expected)
         self.assertEqual(
@@ -1841,6 +1852,7 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
             "voted": False,
             "vote_count": 0,
             "children": [],
+            "editable_fields": ["raw_body", "voted"]
         }
         self.assertEqual(actual, expected)
         self.assertEqual(
@@ -1959,19 +1971,24 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest
             FORUM_ROLE_STUDENT,
         ],
         [True, False],
+        ["question", "discussion"],
         [True, False],
     ))
     @ddt.unpack
-    def test_endorsed_access(self, role_name, is_thread_author, is_comment_author):
+    def test_endorsed_access(self, role_name, is_thread_author, thread_type, is_comment_author):
         role = Role.objects.create(name=role_name, course_id=self.course.id)
         role.users = [self.user]
         self.register_comment(
             {"user_id": str(self.user.id if is_comment_author else (self.user.id + 1))},
             thread_overrides={
-                "user_id": str(self.user.id if is_thread_author else (self.user.id + 1))
+                "thread_type": thread_type,
+                "user_id": str(self.user.id if is_thread_author else (self.user.id + 1)),
             }
         )
-        expected_error = role_name == FORUM_ROLE_STUDENT and not is_thread_author
+        expected_error = (
+            role_name == FORUM_ROLE_STUDENT and
+            (thread_type == "discussion" or not is_thread_author)
+        )
         try:
             update_comment(self.request, "test_comment", {"endorsed": True})
             self.assertFalse(expected_error)

lms/djangoapps/discussion_api/tests/test_permissions.py

+"""
+Tests for discussion API permission logic
+"""
+import itertools
+from unittest import TestCase
+
+import ddt
+
+from discussion_api.permissions import can_delete, get_editable_fields
+from lms.lib.comment_client.comment import Comment
+from lms.lib.comment_client.thread import Thread
+from lms.lib.comment_client.user import User
+
+
+def _get_context(requester_id, is_requester_privileged, thread=None):
+    """Return a context suitable for testing the permissions module"""
+    return {
+        "cc_requester": User(id=requester_id),
+        "is_requester_privileged": is_requester_privileged,
+        "thread": thread,
+    }
+
+
+@ddt.ddt
+class GetEditableFieldsTest(TestCase):
+    """Tests for get_editable_fields"""
+    @ddt.data(*itertools.product([True, False], [True, False]))
+    @ddt.unpack
+    def test_thread(self, is_author, is_privileged):
+        thread = Thread(user_id="5" if is_author else "6", type="thread")
+        context = _get_context(requester_id="5", is_requester_privileged=is_privileged)
+        actual = get_editable_fields(thread, context)
+        expected = {"following", "voted"}
+        if is_author or is_privileged:
+            expected |= {"topic_id", "type", "title", "raw_body"}
+        self.assertEqual(actual, expected)
+
+    @ddt.data(*itertools.product([True, False], [True, False], ["question", "discussion"], [True, False]))
+    @ddt.unpack
+    def test_comment(self, is_author, is_thread_author, thread_type, is_privileged):
+        comment = Comment(user_id="5" if is_author else "6", type="comment")
+        context = _get_context(
+            requester_id="5",
+            is_requester_privileged=is_privileged,
+            thread=Thread(user_id="5" if is_thread_author else "6", thread_type=thread_type)
+        )
+        actual = get_editable_fields(comment, context)
+        expected = {"voted"}
+        if is_author or is_privileged:
+            expected |= {"raw_body"}
+        if (is_thread_author and thread_type == "question") or is_privileged:
+            expected |= {"endorsed"}
+        self.assertEqual(actual, expected)
+
+
+@ddt.ddt
+class CanDeleteTest(TestCase):
+    """Tests for can_delete"""
+    @ddt.data(*itertools.product([True, False], [True, False]))
+    @ddt.unpack
+    def test_thread(self, is_author, is_privileged):
+        thread = Thread(user_id="5" if is_author else "6")
+        context = _get_context(requester_id="5", is_requester_privileged=is_privileged)
+        self.assertEqual(can_delete(thread, context), is_author or is_privileged)
+
+    @ddt.data(*itertools.product([True, False], [True, False], [True, False]))
+    @ddt.unpack
+    def test_comment(self, is_author, is_thread_author, is_privileged):
+        comment = Comment(user_id="5" if is_author else "6")
+        context = _get_context(
+            requester_id="5",
+            is_requester_privileged=is_privileged,
+            thread=Thread(user_id="5" if is_thread_author else "6")
+        )
+        self.assertEqual(can_delete(comment, context), is_author or is_privileged)

lms/djangoapps/discussion_api/tests/test_serializers.py

@@ -151,6 +151,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, ModuleStoreTestCase
 
     def test_basic(self):
         thread = {
+            "type": "thread",
             "id": "test_thread",
             "course_id": unicode(self.course.id),
             "commentable_id": "test_topic",
@@ -196,6 +197,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, ModuleStoreTestCase
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "voted"],
         }
         self.assertEqual(self.serialize(thread), expected)
 
@@ -270,6 +272,7 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase):
 
     def test_basic(self):
         comment = {
+            "type": "comment",
             "id": "test_comment",
             "thread_id": "test_thread",
             "user_id": str(self.author.id),
@@ -302,6 +305,7 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase):
             "voted": False,
             "vote_count": 4,
             "children": [],
+            "editable_fields": ["voted"],
         }
         self.assertEqual(self.serialize(comment), expected)
 

lms/djangoapps/discussion_api/tests/test_views.py

@@ -158,6 +158,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
     def test_basic(self):
         self.register_get_user_response(self.user, upvoted_ids=["test_thread"])
         source_threads = [{
+            "type": "thread",
             "id": "test_thread",
             "course_id": unicode(self.course.id),
             "commentable_id": "test_topic",
@@ -203,6 +204,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "voted"],
         }]
         self.register_get_threads_response(source_threads, page=1, num_pages=2)
         response = self.client.get(self.url, {"course_id": unicode(self.course.id)})
@@ -316,6 +318,7 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"],
         }
         response = self.client.post(
             self.url,
@@ -406,6 +409,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest
             "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread",
             "endorsed_comment_list_url": None,
             "non_endorsed_comment_list_url": None,
+            "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"],
         }
         response = self.client.patch(  # pylint: disable=no-member
             self.url,
@@ -515,6 +519,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
     def test_basic(self):
         self.register_get_user_response(self.user, upvoted_ids=["test_comment"])
         source_comments = [{
+            "type": "comment",
             "id": "test_comment",
             "thread_id": self.thread_id,
             "parent_id": None,
@@ -548,6 +553,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             "voted": True,
             "vote_count": 4,
             "children": [],
+            "editable_fields": ["voted"],
         }]
         self.register_get_thread_response({
             "id": self.thread_id,
@@ -701,6 +707,7 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             "voted": False,
             "vote_count": 0,
             "children": [],
+            "editable_fields": ["raw_body", "voted"],
         }
         response = self.client.post(
             self.url,
@@ -784,6 +791,7 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes
             "voted": False,
             "vote_count": 0,
             "children": [],
+            "editable_fields": ["raw_body", "voted"],
         }
         response = self.client.patch(  # pylint: disable=no-member
             self.url,

lms/djangoapps/discussion_api/tests/utils.py

@@ -273,6 +273,7 @@ def make_minimal_cs_thread(overrides=None):
     comments service with dummy data and optional overrides
     """
     ret = {
+        "type": "thread",
         "id": "dummy",
         "course_id": "dummy/dummy/dummy",
         "commentable_id": "dummy",
@@ -305,6 +306,7 @@ def make_minimal_cs_comment(overrides=None):
     comments service with dummy data and optional overrides
     """
     ret = {
+        "type": "comment",
         "id": "dummy",
         "thread_id": "dummy",
         "parent_id": None,

lms/djangoapps/discussion_api/views.py

@@ -203,6 +203,9 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet):
             that were created or updated since the last time the user read
             the thread
 
+        * editable_fields: The fields that the requesting user is allowed to
+            modify with a PATCH request
+
     **DELETE response values:
 
         No content is returned for a DELETE request
@@ -352,6 +355,9 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet):
 
         * children: The list of child comments (with the same format)
 
+        * editable_fields: The fields that the requesting user is allowed to
+            modify with a PATCH request
+
     **DELETE Response Value**
 
         No content is returned for a DELETE request