Merge pull request #10649 from edx/jia/MA-1190

MA-1190; patch 'read' attribute for thread

lms/djangoapps/discussion_api/api.py

@@ -342,7 +342,7 @@ def get_thread_list(
     return ret
 
 
-def get_comment_list(request, thread_id, endorsed, page, page_size, mark_as_read=False):
+def get_comment_list(request, thread_id, endorsed, page, page_size):
     """
     Return the list of comments in the given thread.
 
@@ -361,8 +361,6 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, mark_as_read
 
         page_size: The number of comments to retrieve per page
 
-        mark_as_read: Marks the thread of the comment list as read.
-
     Returns:
 
         A paginated result containing a list of comments; see
@@ -375,7 +373,6 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, mark_as_read
         retrieve_kwargs={
             "recursive": False,
             "user_id": request.user.id,
-            "mark_as_read": mark_as_read,
             "response_skip": response_skip,
             "response_limit": page_size,
         }

lms/djangoapps/discussion_api/forms.py

@@ -109,7 +109,6 @@ class CommentListGetForm(_PaginationForm):
     # TODO: should we use something better here? This only accepts "True",
     # "False", "1", and "0"
     endorsed = NullBooleanField(required=False)
-    mark_as_read = BooleanField(required=False)
 
 
 class CommentActionsForm(Form):

lms/djangoapps/discussion_api/permissions.py

@@ -57,11 +57,14 @@ def get_editable_fields(cc_content, context):
     Return the set of fields that the requester can edit on the given content
     """
 
-    # no edits, except 'abuse_flagged' are allowed on closed threads
+    # For closed thread:
+    # no edits, except 'abuse_flagged' and 'read' are allowed for thread
+    # no edits, except 'abuse_flagged' is allowed for comment
     ret = {"abuse_flagged"}
-    if (cc_content["type"] == "thread" and cc_content["closed"]) or (
-            cc_content["type"] == "comment" and context["thread"]["closed"]
-    ):
+    if cc_content["type"] == "thread" and cc_content["closed"]:
+        ret |= {"read"}
+        return ret
+    if cc_content["type"] == "comment" and context["thread"]["closed"]:
         return ret
 
     # Shared fields
@@ -71,7 +74,7 @@ def get_editable_fields(cc_content, context):
 
     # Thread fields
     if cc_content["type"] == "thread":
-        ret |= {"following"}
+        ret |= {"following", "read"}
         if _is_author_or_privileged(cc_content, context):
             ret |= {"topic_id", "type", "title"}
         if context["is_requester_privileged"] and context["course"].is_cohorted:

lms/djangoapps/discussion_api/serializers.py

@@ -195,7 +195,7 @@ class ThreadSerializer(_ContentSerializer):
     comment_list_url = serializers.SerializerMethodField()
     endorsed_comment_list_url = serializers.SerializerMethodField()
     non_endorsed_comment_list_url = serializers.SerializerMethodField()
-    read = serializers.BooleanField(read_only=True)
+    read = serializers.BooleanField(required=False)
     has_endorsed = serializers.BooleanField(read_only=True, source="endorsed")
     response_count = serializers.IntegerField(source="resp_total", read_only=True, required=False)
 

lms/djangoapps/discussion_api/tests/test_api.py

@@ -649,7 +649,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
                 "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": ["abuse_flagged", "following", "voted"],
+                "editable_fields": ["abuse_flagged", "following", "read", "voted"],
                 "has_endorsed": True,
                 "read": True,
             },
@@ -682,7 +682,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
                 "non_endorsed_comment_list_url": (
                     "http://testserver/api/discussion/v1/comments/?thread_id=test_thread_id_1&endorsed=False"
                 ),
-                "editable_fields": ["abuse_flagged", "following", "voted"],
+                "editable_fields": ["abuse_flagged", "following", "read", "voted"],
                 "has_endorsed": False,
                 "read": False,
             },
@@ -1402,7 +1402,7 @@ class CreateThreadTest(
             "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": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"],
             'read': False,
             'has_endorsed': False,
             'response_count': 0,
@@ -1948,7 +1948,7 @@ class UpdateThreadTest(
             "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": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"],
             'read': False,
             'has_endorsed': False,
             'response_count': 0
@@ -1967,6 +1967,7 @@ class UpdateThreadTest(
                 "anonymous_to_peers": ["False"],
                 "closed": ["False"],
                 "pinned": ["False"],
+                "read": ["False"],
             }
         )
 
@@ -2989,7 +2990,7 @@ class RetrieveThreadTest(
             "abuse_flagged": False,
             "voted": False,
             "vote_count": 0,
-            "editable_fields": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"],
             "course_id": unicode(self.course.id),
             "topic_id": "test_topic",
             "group_id": None,
@@ -3029,7 +3030,7 @@ class RetrieveThreadTest(
             "abuse_flagged": False,
             "voted": False,
             "vote_count": 0,
-            "editable_fields": ["abuse_flagged", "following", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "read", "voted"],
             "course_id": unicode(self.course.id),
             "topic_id": "test_topic",
             "group_id": None,

lms/djangoapps/discussion_api/tests/test_forms.py

@@ -156,8 +156,7 @@ class CommentListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
                 "thread_id": "deadbeef",
                 "endorsed": False,
                 "page": 2,
-                "page_size": 13,
-                "mark_as_read": False
+                "page_size": 13
             }
         )
 

lms/djangoapps/discussion_api/tests/test_permissions.py

@@ -41,7 +41,7 @@ class GetInitializableFieldsTest(ModuleStoreTestCase):
         )
         actual = get_initializable_thread_fields(context)
         expected = {
-            "abuse_flagged", "course_id", "following", "raw_body", "title", "topic_id", "type", "voted"
+            "abuse_flagged", "course_id", "following", "raw_body", "read", "title", "topic_id", "type", "voted"
         }
         if is_privileged and is_cohorted:
             expected |= {"group_id"}
@@ -77,7 +77,7 @@ class GetEditableFieldsTest(ModuleStoreTestCase):
             is_cohorted=is_cohorted
         )
         actual = get_editable_fields(thread, context)
-        expected = {"abuse_flagged", "following", "voted"}
+        expected = {"abuse_flagged", "following", "read", "voted"}
         if is_author or is_privileged:
             expected |= {"topic_id", "type", "title", "raw_body"}
         if is_privileged and is_cohorted:

lms/djangoapps/discussion_api/tests/test_serializers.py

@@ -208,7 +208,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, SharedModuleStoreTe
             "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": ["abuse_flagged", "following", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "read", "voted"],
             "read": False,
             "has_endorsed": False,
         }
@@ -561,16 +561,19 @@ class ThreadSerializerDeserializationTest(CommentsServiceMockMixin, UrlResetMixi
                 "closed": ["False"],
                 "pinned": ["False"],
                 "user_id": [str(self.user.id)],
+                "read": ["False"],
             }
         )
 
-    def test_update_all(self):
+    @ddt.data(True, False)
+    def test_update_all(self, read):
         self.register_put_thread_response(self.existing_thread.attributes)
         data = {
             "topic_id": "edited_topic",
             "type": "question",
             "title": "Edited Title",
             "raw_body": "Edited body",
+            "read": read,
         }
         saved = self.save_and_reserialize(data, self.existing_thread)
         self.assertEqual(
@@ -586,6 +589,7 @@ class ThreadSerializerDeserializationTest(CommentsServiceMockMixin, UrlResetMixi
                 "closed": ["False"],
                 "pinned": ["False"],
                 "user_id": [str(self.user.id)],
+                "read": [str(read)],
             }
         )
         for key in data:

lms/djangoapps/discussion_api/tests/test_views.py

@@ -301,7 +301,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": ["abuse_flagged", "following", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "read", "voted"],
             "read": False,
             "has_endorsed": False,
         }]
@@ -524,7 +524,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": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"],
             "read": False,
             "has_endorsed": False,
             "response_count": 0,
@@ -619,7 +619,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest
 
     def test_basic(self):
         self.register_get_user_response(self.user)
-        self.register_thread({"created_at": "Test Date", "updated_at": "Test Date"})
+        self.register_thread({"created_at": "Test Created Date", "updated_at": "Test Updated Date"})
         request_data = {"raw_body": "Edited body"}
         response = self.request_patch(request_data)
         self.assertEqual(response.status_code, 200)
@@ -629,9 +629,11 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest
             self.expected_response_data({
                 "raw_body": "Edited body",
                 "rendered_body": "<p>Edited body</p>",
-                "editable_fields": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
-                "created_at": "Test Date",
-                "updated_at": "Test Date",
+                "editable_fields": [
+                    "abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"
+                ],
+                "created_at": "Test Created Date",
+                "updated_at": "Test Updated Date",
             })
         )
         self.assertEqual(
@@ -647,6 +649,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest
                 "anonymous_to_peers": ["False"],
                 "closed": ["False"],
                 "pinned": ["False"],
+                "read": ["False"],
             }
         )
 
@@ -680,7 +683,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest
             self.expected_response_data({
                 "closed": True,
                 "abuse_flagged": value,
-                "editable_fields": ["abuse_flagged"],
+                "editable_fields": ["abuse_flagged", "read"],
             })
         )
 
@@ -1036,7 +1039,7 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes
 
     def test_basic(self):
         self.register_thread()
-        self.register_comment({"created_at": "Test Date", "updated_at": "Test Date"})
+        self.register_comment({"created_at": "Test Created Date", "updated_at": "Test Updated Date"})
         request_data = {"raw_body": "Edited body"}
         response = self.request_patch(request_data)
         self.assertEqual(response.status_code, 200)
@@ -1047,8 +1050,8 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes
                 "raw_body": "Edited body",
                 "rendered_body": "<p>Edited body</p>",
                 "editable_fields": ["abuse_flagged", "raw_body", "voted"],
-                "created_at": "Test Date",
-                "updated_at": "Test Date",
+                "created_at": "Test Created Date",
+                "updated_at": "Test Updated Date",
             })
         )
         self.assertEqual(
@@ -1142,7 +1145,7 @@ class ThreadViewSetRetrieveTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase)
             "abuse_flagged": False,
             "voted": False,
             "vote_count": 0,
-            "editable_fields": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"],
+            "editable_fields": ["abuse_flagged", "following", "raw_body", "read", "title", "topic_id", "type", "voted"],
             "course_id": unicode(self.course.id),
             "topic_id": "test_topic",
             "group_id": None,

lms/djangoapps/discussion_api/views.py

@@ -104,13 +104,15 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
     """
     **Use Cases**
 
-        Retrieve the list of threads for a course, post a new thread, or modify
-        or delete an existing thread.
+        Retrieve the list of threads for a course, retrieve thread details,
+        post a new thread, or modify or delete an existing thread.
 
     **Example Requests**:
 
         GET /api/discussion/v1/threads/?course_id=ExampleX/Demo/2015
 
+        GET /api/discussion/v1/threads/thread_id
+
         POST /api/discussion/v1/threads
         {
           "course_id": "foo/bar/baz",
@@ -126,7 +128,7 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
 
         DELETE /api/discussion/v1/threads/thread_id
 
-    **GET Parameters**:
+    **GET Thread List Parameters**:
 
         * course_id (required): The course to retrieve threads for
 
@@ -176,13 +178,19 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
 
     **PATCH Parameters**:
 
-        topic_id, type, title, and raw_body are accepted with the same meaning
+        * abuse_flagged (optional): A boolean to mark thread as abusive
+
+        * voted (optional): A boolean to vote for thread
+
+        * read (optional): A boolean to mark thread as read
+
+        * topic_id, type, title, and raw_body are accepted with the same meaning
         as in a POST request
 
         If "application/merge-patch+json" is not the specified content type,
         a 415 error is returned.
 
-    **GET Response Values**:
+    **GET Thread List Response Values**:
 
         * results: The list of threads; each item in the list has the same
             fields as the POST/PATCH response below
@@ -195,6 +203,10 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
             parameter was rewritten in order to match threads (e.g. for spelling
             correction)
 
+    **GET Thread Details Response Values**:
+
+        Same response fields as the POST/PATCH response below
+
     **POST/PATCH response values**:
 
         * id: The id of the thread
@@ -300,7 +312,8 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet):
     """
     **Use Cases**
 
-        Retrieve the list of comments in a thread, create a comment, or modify
+        Retrieve the list of comments in a thread, retrieve the list of
+        child comments for a response comment, create a comment, or modify
         or delete an existing comment.
 
     **Example Requests**:
@@ -333,9 +346,6 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet):
 
         * page_size: The number of items per page (default is 10, max is 100)
 
-        * mark_as_read: Will mark the thread of the comments as read. (default
-            is False)
-
     **GET Child Comment List Parameters**:
 
         * comment_id (required): The comment to retrieve child comments for
@@ -439,8 +449,7 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet):
                 form.cleaned_data["thread_id"],
                 form.cleaned_data["endorsed"],
                 form.cleaned_data["page"],
-                form.cleaned_data["page_size"],
-                form.cleaned_data["mark_as_read"]
+                form.cleaned_data["page_size"]
             )
         )
 

lms/lib/comment_client/thread.py

@@ -25,7 +25,7 @@ class Thread(models.Model):
 
     # updateable_fields are sent in PUT requests
     updatable_fields = [
-        'title', 'body', 'anonymous', 'anonymous_to_peers', 'course_id',
+        'title', 'body', 'anonymous', 'anonymous_to_peers', 'course_id', 'read',
         'closed', 'user_id', 'commentable_id', 'group_id', 'group_name', 'pinned', 'thread_type'
     ]