diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index ae9e4f27a1..1e0923bdf6 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -601,6 +601,10 @@ def create_comment(request, comment_data): except Http404: raise ValidationError({"thread_id": ["Invalid value."]}) + # if a thread is closed; no new comments could be made to it + if cc_thread['closed']: + raise PermissionDenied + _check_initializable_comment_fields(comment_data, context) serializer = CommentSerializer(data=comment_data, context=context) actions_form = CommentActionsForm(comment_data) diff --git a/lms/djangoapps/discussion_api/permissions.py b/lms/djangoapps/discussion_api/permissions.py index 0b77be202e..4385c070a3 100644 --- a/lms/djangoapps/discussion_api/permissions.py +++ b/lms/djangoapps/discussion_api/permissions.py @@ -56,8 +56,16 @@ 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 + ret = {"abuse_flagged"} + if (cc_content["type"] == "thread" and cc_content["closed"]) or ( + cc_content["type"] == "comment" and context["thread"]["closed"] + ): + return ret + # Shared fields - ret = {"abuse_flagged", "voted"} + ret |= {"voted"} if _is_author_or_privileged(cc_content, context): ret |= {"raw_body"} diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index bc640b4780..0bd5968657 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -614,7 +614,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto "title": "Another Test Title", "body": "More content", "pinned": False, - "closed": True, + "closed": False, "abuse_flaggers": [], "votes": {"up_count": 9}, "comments_count": 18, @@ -668,7 +668,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto "raw_body": "More content", "rendered_body": "

More content

", "pinned": False, - "closed": True, + "closed": False, "following": False, "abuse_flagged": False, "voted": False, diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 31737d4ae0..35ab192577 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -63,6 +63,41 @@ class DiscussionAPIViewTestMixin(CommentsServiceMockMixin, UrlResetMixin): parsed_content = json.loads(response.content) self.assertEqual(parsed_content, expected_content) + def register_thread(self, overrides=None): + """ + Create cs_thread with minimal fields and register response + """ + cs_thread = make_minimal_cs_thread({ + "id": "test_thread", + "course_id": unicode(self.course.id), + "commentable_id": "original_topic", + "username": self.user.username, + "user_id": str(self.user.id), + "thread_type": "discussion", + "title": "Original Title", + "body": "Original body", + }) + cs_thread.update(overrides or {}) + self.register_get_thread_response(cs_thread) + self.register_put_thread_response(cs_thread) + + def register_comment(self, overrides=None): + """ + Create cs_comment with minimal fields and register response + """ + cs_comment = make_minimal_cs_comment({ + "id": "test_comment", + "course_id": unicode(self.course.id), + "thread_id": "test_thread", + "username": self.user.username, + "user_id": str(self.user.id), + "body": "Original body", + }) + cs_comment.update(overrides or {}) + self.register_get_comment_response(cs_comment) + self.register_put_comment_response(cs_comment) + self.register_post_comment_response(cs_comment, thread_id="test_thread") + def test_not_authenticated(self): self.client.logout() response = self.client.get(self.url) @@ -534,6 +569,7 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assertEqual(response_data, expected_response_data) +@ddt.ddt @httpretty.activate @disable_signal(api, 'thread_edited') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -544,24 +580,11 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest super(ThreadViewSetPartialUpdateTest, self).setUp() self.url = reverse("thread-detail", kwargs={"thread_id": "test_thread"}) - def test_basic(self): - self.register_get_user_response(self.user) - cs_thread = make_minimal_cs_thread({ - "id": "test_thread", - "course_id": unicode(self.course.id), - "commentable_id": "original_topic", - "username": self.user.username, - "user_id": str(self.user.id), - "created_at": "2015-05-29T00:00:00Z", - "updated_at": "2015-05-29T00:00:00Z", - "thread_type": "discussion", - "title": "Original Title", - "body": "Original body", - }) - self.register_get_thread_response(cs_thread) - self.register_put_thread_response(cs_thread) - request_data = {"raw_body": "Edited body"} - expected_response_data = { + def expected_response_data(self, overrides=None): + """ + create expected response data from comment update endpoint + """ + response_data = { "id": "test_thread", "course_id": unicode(self.course.id), "topic_id": "original_topic", @@ -569,12 +592,12 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest "group_name": None, "author": self.user.username, "author_label": None, - "created_at": "2015-05-29T00:00:00Z", - "updated_at": "2015-05-29T00:00:00Z", + "created_at": "1970-01-01T00:00:00Z", + "updated_at": "1970-01-01T00:00:00Z", "type": "discussion", "title": "Original Title", - "raw_body": "Edited body", - "rendered_body": "

Edited body

", + "raw_body": "Original body", + "rendered_body": "

Original body

", "pinned": False, "closed": False, "following": False, @@ -586,19 +609,31 @@ 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": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"], + "editable_fields": [], "read": False, "has_endorsed": False, "response_count": 0, } - response = self.client.patch( # pylint: disable=no-member - self.url, - json.dumps(request_data), - content_type="application/merge-patch+json" - ) + response_data.update(overrides or {}) + return response_data + + def test_basic(self): + self.register_get_user_response(self.user) + self.register_thread({"created_at": "Test Date", "updated_at": "Test Date"}) + request_data = {"raw_body": "Edited body"} + response = self.request_patch(request_data) self.assertEqual(response.status_code, 200) response_data = json.loads(response.content) - self.assertEqual(response_data, expected_response_data) + self.assertEqual( + response_data, + self.expected_response_data({ + "raw_body": "Edited body", + "rendered_body": "

Edited body

", + "editable_fields": ["abuse_flagged", "following", "raw_body", "title", "topic_id", "type", "voted"], + "created_at": "Test Date", + "updated_at": "Test Date", + }) + ) self.assertEqual( httpretty.last_request().parsed_body, { @@ -617,18 +652,9 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest def test_error(self): self.register_get_user_response(self.user) - cs_thread = make_minimal_cs_thread({ - "id": "test_thread", - "course_id": unicode(self.course.id), - "user_id": str(self.user.id), - }) - self.register_get_thread_response(cs_thread) + self.register_thread() request_data = {"title": ""} - response = self.client.patch( # pylint: disable=no-member - self.url, - json.dumps(request_data), - content_type="application/merge-patch+json" - ) + response = self.request_patch(request_data) expected_response_data = { "field_errors": {"title": {"developer_message": "This field may not be blank."}} } @@ -636,6 +662,42 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest response_data = json.loads(response.content) self.assertEqual(response_data, expected_response_data) + @ddt.data( + ("abuse_flagged", True), + ("abuse_flagged", False), + ) + @ddt.unpack + def test_closed_thread(self, field, value): + self.register_get_user_response(self.user) + self.register_thread({"closed": True}) + self.register_flag_response("thread", "test_thread") + request_data = {field: value} + response = self.request_patch(request_data) + self.assertEqual(response.status_code, 200) + response_data = json.loads(response.content) + self.assertEqual( + response_data, + self.expected_response_data({ + "closed": True, + "abuse_flagged": value, + "editable_fields": ["abuse_flagged"], + }) + ) + + @ddt.data( + ("raw_body", "Edited body"), + ("voted", True), + ("following", True), + ) + @ddt.unpack + def test_closed_thread_error(self, field, value): + self.register_get_user_response(self.user) + self.register_thread({"closed": True}) + self.register_flag_response("thread", "test_thread") + request_data = {field: value} + response = self.request_patch(request_data) + self.assertEqual(response.status_code, 400) + @httpretty.activate @disable_signal(api, 'thread_deleted') @@ -855,22 +917,8 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): def test_basic(self): self.register_get_user_response(self.user) - self.register_get_thread_response( - make_minimal_cs_thread({ - "id": "test_thread", - "course_id": unicode(self.course.id), - "commentable_id": "test_topic", - }) - ) - self.register_post_comment_response( - { - "id": "test_comment", - "username": self.user.username, - "created_at": "2015-05-27T00:00:00Z", - "updated_at": "2015-05-27T00:00:00Z", - }, - thread_id="test_thread" - ) + self.register_thread() + self.register_comment() request_data = { "thread_id": "test_thread", "raw_body": "Test body", @@ -881,8 +929,8 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "parent_id": None, "author": self.user.username, "author_label": None, - "created_at": "2015-05-27T00:00:00Z", - "updated_at": "2015-05-27T00:00:00Z", + "created_at": "1970-01-01T00:00:00Z", + "updated_at": "1970-01-01T00:00:00Z", "raw_body": "Test body", "rendered_body": "

Test body

", "endorsed": False, @@ -929,7 +977,23 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): response_data = json.loads(response.content) self.assertEqual(response_data, expected_response_data) + def test_closed_thread(self): + self.register_get_user_response(self.user) + self.register_thread({"closed": True}) + self.register_comment() + request_data = { + "thread_id": "test_thread", + "raw_body": "Test body" + } + response = self.client.post( + self.url, + json.dumps(request_data), + content_type="application/json" + ) + self.assertEqual(response.status_code, 403) + +@ddt.ddt @disable_signal(api, 'comment_edited') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, PatchMediaTypeMixin): @@ -942,36 +1006,21 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes self.addCleanup(httpretty.disable) self.register_get_user_response(self.user) self.url = reverse("comment-detail", kwargs={"comment_id": "test_comment"}) - cs_thread = make_minimal_cs_thread({ - "id": "test_thread", - "course_id": unicode(self.course.id), - }) - self.register_get_thread_response(cs_thread) - cs_comment = make_minimal_cs_comment({ - "id": "test_comment", - "course_id": cs_thread["course_id"], - "thread_id": cs_thread["id"], - "username": self.user.username, - "user_id": str(self.user.id), - "created_at": "2015-06-03T00:00:00Z", - "updated_at": "2015-06-03T00:00:00Z", - "body": "Original body", - }) - self.register_get_comment_response(cs_comment) - self.register_put_comment_response(cs_comment) - def test_basic(self): - request_data = {"raw_body": "Edited body"} - expected_response_data = { + def expected_response_data(self, overrides=None): + """ + create expected response data from comment update endpoint + """ + response_data = { "id": "test_comment", "thread_id": "test_thread", "parent_id": None, "author": self.user.username, "author_label": None, - "created_at": "2015-06-03T00:00:00Z", - "updated_at": "2015-06-03T00:00:00Z", - "raw_body": "Edited body", - "rendered_body": "

Edited body

", + "created_at": "1970-01-01T00:00:00Z", + "updated_at": "1970-01-01T00:00:00Z", + "raw_body": "Original body", + "rendered_body": "

Original body

", "endorsed": False, "endorsed_by": None, "endorsed_by_label": None, @@ -980,16 +1029,28 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes "voted": False, "vote_count": 0, "children": [], - "editable_fields": ["abuse_flagged", "raw_body", "voted"], + "editable_fields": [], } - response = self.client.patch( # pylint: disable=no-member - self.url, - json.dumps(request_data), - content_type="application/merge-patch+json" - ) + response_data.update(overrides or {}) + return response_data + + def test_basic(self): + self.register_thread() + self.register_comment({"created_at": "Test Date", "updated_at": "Test Date"}) + request_data = {"raw_body": "Edited body"} + response = self.request_patch(request_data) self.assertEqual(response.status_code, 200) response_data = json.loads(response.content) - self.assertEqual(response_data, expected_response_data) + self.assertEqual( + response_data, + self.expected_response_data({ + "raw_body": "Edited body", + "rendered_body": "

Edited body

", + "editable_fields": ["abuse_flagged", "raw_body", "voted"], + "created_at": "Test Date", + "updated_at": "Test Date", + }) + ) self.assertEqual( httpretty.last_request().parsed_body, { @@ -1003,12 +1064,10 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes ) def test_error(self): + self.register_thread() + self.register_comment() request_data = {"raw_body": ""} - response = self.client.patch( # pylint: disable=no-member - self.url, - json.dumps(request_data), - content_type="application/merge-patch+json" - ) + response = self.request_patch(request_data) expected_response_data = { "field_errors": {"raw_body": {"developer_message": "This field may not be blank."}} } @@ -1016,6 +1075,40 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes response_data = json.loads(response.content) self.assertEqual(response_data, expected_response_data) + @ddt.data( + ("abuse_flagged", True), + ("abuse_flagged", False), + ) + @ddt.unpack + def test_closed_thread(self, field, value): + self.register_thread({"closed": True}) + self.register_comment() + self.register_flag_response("comment", "test_comment") + request_data = {field: value} + response = self.request_patch(request_data) + self.assertEqual(response.status_code, 200) + response_data = json.loads(response.content) + self.assertEqual( + response_data, + self.expected_response_data({ + "abuse_flagged": value, + "editable_fields": ["abuse_flagged"], + }) + ) + + @ddt.data( + ("raw_body", "Edited body"), + ("voted", True), + ("following", True), + ) + @ddt.unpack + def test_closed_thread_error(self, field, value): + self.register_thread({"closed": True}) + self.register_comment() + request_data = {field: value} + response = self.request_patch(request_data) + self.assertEqual(response.status_code, 400) + @httpretty.activate @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 6a7da90aaa..322546c619 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -301,6 +301,16 @@ class CommentsServiceMockMixin(object): """ self.assert_query_params_equal(httpretty.last_request(), expected_params) + def request_patch(self, request_data): + """ + make a request to PATCH endpoint and return response + """ + return self.client.patch( # pylint: disable=no-member + self.url, + json.dumps(request_data), + content_type="application/merge-patch+json" + ) + def make_minimal_cs_thread(overrides=None): """