diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index f8a5f6288c51..cb6ff4ea9673 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -90,6 +90,7 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se # For closed thread: # no edits, except 'abuse_flagged' and 'read' are allowed for thread # no edits, except 'abuse_flagged' is allowed for comment + is_thread = cc_content["type"] == "thread" is_comment = cc_content["type"] == "comment" has_moderation_privilege = context["has_moderation_privilege"] @@ -120,7 +121,7 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se is_author = _is_author(cc_content, context) editable_fields.update({ - "voted": True, + "voted": has_moderation_privilege or not is_author or is_staff_or_admin, "raw_body": has_moderation_privilege or is_author, "edit_reason_code": has_moderation_privilege and not is_author, "following": is_thread, diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 5e34b688d15f..9a9041fd5fa4 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -2128,19 +2128,6 @@ def test_following(self): assert cs_request.method == 'POST' assert parsed_body(cs_request) == {'source_type': ['thread'], 'source_id': ['test_id']} - def test_voted(self): - self.register_post_thread_response({"id": "test_id", "username": self.user.username}) - self.register_thread_votes_response("test_id") - data = self.minimal_data.copy() - data["voted"] = "True" - with self.assert_signal_sent(api, 'thread_voted', sender=None, user=self.user, exclude_args=('post',)): - result = create_thread(self.request, data) - assert result['voted'] is True - cs_request = httpretty.last_request() - assert urlparse(cs_request.path).path == '/api/v1/threads/test_id/votes' # lint-amnesty, pylint: disable=no-member - assert cs_request.method == 'PUT' - assert parsed_body(cs_request) == {'user_id': [str(self.user.id)], 'value': ['up']} - def test_abuse_flagged(self): self.register_post_thread_response({"id": "test_id", "username": self.user.username}) self.register_thread_flag_response("test_id") @@ -2278,7 +2265,7 @@ def test_success(self, parent_id, mock_emit): "voted": False, "vote_count": 0, "children": [], - "editable_fields": ["abuse_flagged", "anonymous", "raw_body", "voted"], + "editable_fields": ["abuse_flagged", "anonymous", "raw_body"], "child_count": 0, "can_delete": True, "anonymous": False, @@ -2354,7 +2341,7 @@ def test_success_in_black_out_with_user_access(self, parent_id, mock_emit): "abuse_flagged", "anonymous", "raw_body", - "voted", + "voted" ] if parent_id: data["parent_id"] = parent_id @@ -2485,19 +2472,6 @@ def test_endorsed(self, role_name, is_thread_author, thread_type): except ValidationError: assert expected_error - def test_voted(self): - self.register_post_comment_response({"id": "test_comment", "username": self.user.username}, "test_thread") - self.register_comment_votes_response("test_comment") - data = self.minimal_data.copy() - data["voted"] = "True" - with self.assert_signal_sent(api, 'comment_voted', sender=None, user=self.user, exclude_args=('post',)): - result = create_comment(self.request, data) - assert result['voted'] is True - cs_request = httpretty.last_request() - assert urlparse(cs_request.path).path == '/api/v1/comments/test_comment/votes' # lint-amnesty, pylint: disable=no-member - assert cs_request.method == 'PUT' - assert parsed_body(cs_request) == {'user_id': [str(self.user.id)], 'value': ['up']} - def test_abuse_flagged(self): self.register_post_comment_response({"id": "test_comment", "username": self.user.username}, "test_thread") self.register_comment_flag_response("test_comment") @@ -2642,6 +2616,17 @@ def register_thread(self, overrides=None): self.register_get_thread_response(cs_data) self.register_put_thread_response(cs_data) + def create_user_with_request(self): + """ + Create a user and an associated request for a specific course enrollment. + """ + user = UserFactory.create() + self.register_get_user_response(user) + request = RequestFactory().get("/test_path") + request.user = user + CourseEnrollmentFactory.create(user=user, course_id=self.course.id) + return user, request + def test_empty(self): """Check that an empty update does not make any modifying requests.""" # Ensure that the default following value of False is not applied implicitly @@ -2813,12 +2798,15 @@ def test_voted(self, current_vote_status, new_vote_status, mock_emit): are the same, no update should be made. Otherwise, a vote should be PUT or DELETEd according to the new_vote_status value. """ + #setup + user1, request1 = self.create_user_with_request() + if current_vote_status: - self.register_get_user_response(self.user, upvoted_ids=["test_thread"]) + self.register_get_user_response(user1, upvoted_ids=["test_thread"]) self.register_thread_votes_response("test_thread") self.register_thread() data = {"voted": new_vote_status} - result = update_thread(self.request, "test_thread", data) + result = update_thread(request1, "test_thread", data) assert result['voted'] == new_vote_status last_request_path = urlparse(httpretty.last_request().path).path # lint-amnesty, pylint: disable=no-member votes_url = "/api/v1/threads/test_thread/votes" @@ -2832,7 +2820,7 @@ def test_voted(self, current_vote_status, new_vote_status, mock_emit): parse_qs(urlparse(httpretty.last_request().path).query) # lint-amnesty, pylint: disable=no-member ) actual_request_data.pop("request_id", None) - expected_request_data = {"user_id": [str(self.user.id)]} + expected_request_data = {"user_id": [str(user1.id)]} if new_vote_status: expected_request_data["value"] = ["up"] assert actual_request_data == expected_request_data @@ -2858,21 +2846,22 @@ def test_vote_count(self, current_vote_status, first_vote, second_vote): """ #setup starting_vote_count = 0 + user, request = self.create_user_with_request() if current_vote_status: - self.register_get_user_response(self.user, upvoted_ids=["test_thread"]) + self.register_get_user_response(user, upvoted_ids=["test_thread"]) starting_vote_count = 1 self.register_thread_votes_response("test_thread") self.register_thread(overrides={"votes": {"up_count": starting_vote_count}}) #first vote data = {"voted": first_vote} - result = update_thread(self.request, "test_thread", data) + result = update_thread(request, "test_thread", data) self.register_thread(overrides={"voted": first_vote}) assert result['vote_count'] == (1 if first_vote else 0) #second vote data = {"voted": second_vote} - result = update_thread(self.request, "test_thread", data) + result = update_thread(request, "test_thread", data) assert result['vote_count'] == (1 if second_vote else 0) @ddt.data(*itertools.product([True, False], [True, False], [True, False], [True, False])) @@ -2888,22 +2877,19 @@ def test_vote_count_two_users( Tests vote_count increases and decreases correctly from different users """ #setup - user2 = UserFactory.create() - self.register_get_user_response(user2) - request2 = RequestFactory().get("/test_path") - request2.user = user2 - CourseEnrollmentFactory.create(user=user2, course_id=self.course.id) + user1, request1 = self.create_user_with_request() + user2, request2 = self.create_user_with_request() vote_count = 0 if current_user1_vote: - self.register_get_user_response(self.user, upvoted_ids=["test_thread"]) + self.register_get_user_response(user1, upvoted_ids=["test_thread"]) vote_count += 1 if current_user2_vote: self.register_get_user_response(user2, upvoted_ids=["test_thread"]) vote_count += 1 for (current_vote, user_vote, request) in \ - [(current_user1_vote, user1_vote, self.request), + [(current_user1_vote, user1_vote, request1), (current_user2_vote, user2_vote, request2)]: self.register_thread_votes_response("test_thread") @@ -3202,6 +3188,17 @@ def register_comment(self, overrides=None, thread_overrides=None, course=None): self.register_get_comment_response(cs_comment_data) self.register_put_comment_response(cs_comment_data) + def create_user_with_request(self): + """ + Create a user and an associated request for a specific course enrollment. + """ + user = UserFactory.create() + self.register_get_user_response(user) + request = RequestFactory().get("/test_path") + request.user = user + CourseEnrollmentFactory.create(user=user, course_id=self.course.id) + return user, request + def test_empty(self): """Check that an empty update does not make any modifying requests.""" self.register_comment() @@ -3235,7 +3232,7 @@ def test_basic(self, parent_id): "voted": False, "vote_count": 0, "children": [], - "editable_fields": ["abuse_flagged", "anonymous", "raw_body", "voted"], + "editable_fields": ["abuse_flagged", "anonymous", "raw_body"], "child_count": 0, "can_delete": True, "last_edit": None, @@ -3394,13 +3391,14 @@ def test_voted(self, current_vote_status, new_vote_status, mock_emit): or DELETEd according to the new_vote_status value. """ vote_count = 0 + user1, request1 = self.create_user_with_request() if current_vote_status: - self.register_get_user_response(self.user, upvoted_ids=["test_comment"]) + self.register_get_user_response(user1, upvoted_ids=["test_comment"]) vote_count = 1 self.register_comment_votes_response("test_comment") self.register_comment(overrides={"votes": {"up_count": vote_count}}) data = {"voted": new_vote_status} - result = update_comment(self.request, "test_comment", data) + result = update_comment(request1, "test_comment", data) assert result['vote_count'] == (1 if new_vote_status else 0) assert result['voted'] == new_vote_status last_request_path = urlparse(httpretty.last_request().path).path # lint-amnesty, pylint: disable=no-member @@ -3415,7 +3413,7 @@ def test_voted(self, current_vote_status, new_vote_status, mock_emit): parse_qs(urlparse(httpretty.last_request().path).query) # lint-amnesty, pylint: disable=no-member ) actual_request_data.pop("request_id", None) - expected_request_data = {"user_id": [str(self.user.id)]} + expected_request_data = {"user_id": [str(user1.id)]} if new_vote_status: expected_request_data["value"] = ["up"] assert actual_request_data == expected_request_data @@ -3442,21 +3440,22 @@ def test_vote_count(self, current_vote_status, first_vote, second_vote): """ #setup starting_vote_count = 0 + user1, request1 = self.create_user_with_request() if current_vote_status: - self.register_get_user_response(self.user, upvoted_ids=["test_comment"]) + self.register_get_user_response(user1, upvoted_ids=["test_comment"]) starting_vote_count = 1 self.register_comment_votes_response("test_comment") self.register_comment(overrides={"votes": {"up_count": starting_vote_count}}) #first vote data = {"voted": first_vote} - result = update_comment(self.request, "test_comment", data) + result = update_comment(request1, "test_comment", data) self.register_comment(overrides={"voted": first_vote}) assert result['vote_count'] == (1 if first_vote else 0) #second vote data = {"voted": second_vote} - result = update_comment(self.request, "test_comment", data) + result = update_comment(request1, "test_comment", data) assert result['vote_count'] == (1 if second_vote else 0) @ddt.data(*itertools.product([True, False], [True, False], [True, False], [True, False])) @@ -3471,22 +3470,19 @@ def test_vote_count_two_users( """ Tests vote_count increases and decreases correctly from different users """ - user2 = UserFactory.create() - self.register_get_user_response(user2) - request2 = RequestFactory().get("/test_path") - request2.user = user2 - CourseEnrollmentFactory.create(user=user2, course_id=self.course.id) + user1, request1 = self.create_user_with_request() + user2, request2 = self.create_user_with_request() vote_count = 0 if current_user1_vote: - self.register_get_user_response(self.user, upvoted_ids=["test_comment"]) + self.register_get_user_response(user1, upvoted_ids=["test_comment"]) vote_count += 1 if current_user2_vote: self.register_get_user_response(user2, upvoted_ids=["test_comment"]) vote_count += 1 for (current_vote, user_vote, request) in \ - [(current_user1_vote, user1_vote, self.request), + [(current_user1_vote, user1_vote, request1), (current_user2_vote, user2_vote, request2)]: self.register_comment_votes_response("test_comment") diff --git a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py index 756229150e30..405726e2125b 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py @@ -66,10 +66,10 @@ def test_thread( actual = get_initializable_thread_fields(context) expected = { "abuse_flagged", "copy_link", "course_id", "following", "raw_body", - "read", "title", "topic_id", "type", "voted" + "read", "title", "topic_id", "type" } if is_privileged: - expected |= {"closed", "pinned", "close_reason_code"} + expected |= {"closed", "pinned", "close_reason_code", "voted"} if is_privileged and is_cohorted: expected |= {"group_id"} if allow_anonymous: @@ -88,8 +88,10 @@ def test_comment(self, is_thread_author, thread_type, is_privileged): ) actual = get_initializable_comment_fields(context) expected = { - "anonymous", "abuse_flagged", "parent_id", "raw_body", "thread_id", "voted" + "anonymous", "abuse_flagged", "parent_id", "raw_body", "thread_id" } + if is_privileged: + expected |= {"voted"} if (is_thread_author and thread_type == "question") or is_privileged: expected |= {"endorsed"} assert actual == expected @@ -119,11 +121,13 @@ def test_thread( is_staff_or_admin=is_staff_or_admin, ) actual = get_editable_fields(thread, context) - expected = {"abuse_flagged", "copy_link", "following", "read", "voted"} + expected = {"abuse_flagged", "copy_link", "following", "read"} if has_moderation_privilege: expected |= {"closed", "close_reason_code"} if has_moderation_privilege or is_staff_or_admin: expected |= {"pinned"} + if has_moderation_privilege or not is_author or is_staff_or_admin: + expected |= {"voted"} if has_moderation_privilege and not is_author: expected |= {"edit_reason_code"} if is_author or has_moderation_privilege: @@ -162,7 +166,9 @@ def test_comment( has_moderation_privilege=has_moderation_privilege, ) actual = get_editable_fields(comment, context) - expected = {"abuse_flagged", "voted"} + expected = {"abuse_flagged"} + if has_moderation_privilege or not is_author: + expected |= {"voted"} if has_moderation_privilege and not is_author: expected |= {"edit_reason_code"} if is_author or has_moderation_privilege: diff --git a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py index 5d958370c16a..8103eb692791 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py @@ -143,6 +143,7 @@ def test_voted(self): @ddt.ddt class ThreadSerializerSerializationTest(SerializerTestMixin, SharedModuleStoreTestCase): """Tests for ThreadSerializer serialization.""" + def make_cs_content(self, overrides): """ Create a thread with the given overrides, plus some useful test data. @@ -279,6 +280,7 @@ def test_closed_by_label_field(self, role, visible): can_delete = role != FORUM_ROLE_STUDENT editable_fields = ["abuse_flagged", "copy_link", "following", "read", "voted"] if role == "author": + editable_fields.remove("voted") editable_fields.extend(['anonymous', 'raw_body', 'title', 'topic_id', 'type']) elif role == FORUM_ROLE_MODERATOR: editable_fields.extend(['close_reason_code', 'closed', 'edit_reason_code', 'pinned', @@ -335,7 +337,9 @@ def test_edit_by_label_field(self, role, visible): editable_fields = ["abuse_flagged", "copy_link", "following", "read", "voted"] if role == "author": + editable_fields.remove("voted") editable_fields.extend(['anonymous', 'raw_body', 'title', 'topic_id', 'type']) + elif role == FORUM_ROLE_MODERATOR: editable_fields.extend(['close_reason_code', 'closed', 'edit_reason_code', 'pinned', 'raw_body', 'title', 'topic_id', 'type']) @@ -375,6 +379,7 @@ def test_get_preview_body(self): @ddt.ddt class CommentSerializerTest(SerializerTestMixin, SharedModuleStoreTestCase): """Tests for CommentSerializer.""" + def setUp(self): super().setUp() self.endorser = UserFactory.create() @@ -610,7 +615,7 @@ def test_create_minimal(self): self.register_post_thread_response({"id": "test_id", "username": self.user.username}) saved = self.save_and_reserialize(self.minimal_data) assert urlparse(httpretty.last_request().path).path ==\ - '/api/v1/test_topic/threads' # lint-amnesty, pylint: disable=no-member + '/api/v1/test_topic/threads' # lint-amnesty, pylint: disable=no-member assert parsed_body(httpretty.last_request()) == { 'course_id': [str(self.course.id)], 'commentable_id': ['test_topic'], diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index d7bf6c0e139e..283117000712 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -1456,7 +1456,7 @@ def test_basic(self): 'preview_body': 'Edited body', 'editable_fields': [ 'abuse_flagged', 'anonymous', 'copy_link', 'following', 'raw_body', 'read', - 'title', 'topic_id', 'type', 'voted' + 'title', 'topic_id', 'type' ], 'created_at': 'Test Created Date', 'updated_at': 'Test Updated Date', @@ -1540,7 +1540,7 @@ def test_patch_read_owner_user(self): 'read': True, 'editable_fields': [ 'abuse_flagged', 'anonymous', 'copy_link', 'following', 'raw_body', 'read', - 'title', 'topic_id', 'type', 'voted' + 'title', 'topic_id', 'type' ], 'response_count': 2 }) @@ -1647,7 +1647,7 @@ def setUp(self): "key": "editable_fields", "value": [ 'abuse_flagged', 'anonymous', 'copy_link', 'following', 'raw_body', - 'read', 'title', 'topic_id', 'type', 'voted' + 'read', 'title', 'topic_id', 'type' ] }, {"key": "endorsed_comment_list_url", "value": None}, @@ -2444,7 +2444,7 @@ def test_basic(self): "voted": False, "vote_count": 0, "children": [], - "editable_fields": ["abuse_flagged", "anonymous", "raw_body", "voted"], + "editable_fields": ["abuse_flagged", "anonymous", "raw_body"], "child_count": 0, "can_delete": True, "anonymous": False, @@ -2572,7 +2572,7 @@ def test_basic(self): assert response_data == self.expected_response_data({ 'raw_body': 'Edited body', 'rendered_body': '

Edited body

', - 'editable_fields': ['abuse_flagged', 'anonymous', 'raw_body', 'voted'], + 'editable_fields': ['abuse_flagged', 'anonymous', 'raw_body'], 'created_at': 'Test Created Date', 'updated_at': 'Test Updated Date' }) @@ -2743,7 +2743,7 @@ def test_basic(self): "vote_count": 0, "abuse_flagged": False, "abuse_flagged_any_user": None, - "editable_fields": ["abuse_flagged", "anonymous", "raw_body", "voted"], + "editable_fields": ["abuse_flagged", "anonymous", "raw_body"], "child_count": 0, "can_delete": True, "anonymous": False, diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index 39dddcf33f3f..989fd63855d5 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -88,6 +88,7 @@ def callback(request, _uri, headers): class CommentsServiceMockMixin: """Mixin with utility methods for mocking the comments service""" + def register_get_threads_response(self, threads, page, num_pages): """Register a mock response for GET on the CS thread list endpoint""" assert httpretty.is_enabled(), 'httpretty must be enabled to mock calls.' @@ -489,7 +490,6 @@ def expected_thread_data(self, overrides=None): "title", "topic_id", "type", - "voted", ], "course_id": str(self.course.id), "topic_id": "test_topic",