diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index b5abaa9ce9..6102948db8 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -4,7 +4,6 @@ Discussion API internal interface import itertools from collections import defaultdict -from enum import Enum from typing import List, Literal, Optional from urllib.parse import urlencode, urlunparse @@ -12,6 +11,7 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.http import Http404 from django.urls import reverse +from enum import Enum from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey from rest_framework.exceptions import PermissionDenied @@ -103,13 +103,10 @@ def _get_course(course_key, user): """ try: course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) - except Http404: + except (Http404, CourseAccessRedirect) as err: # Convert 404s into CourseNotFoundErrors. - raise CourseNotFoundError("Course not found.") # lint-amnesty, pylint: disable=raise-missing-from - except CourseAccessRedirect: # Raise course not found if the user cannot access the course - # since it doesn't make sense to redirect an API. - raise CourseNotFoundError("Course not found.") # lint-amnesty, pylint: disable=raise-missing-from + raise CourseNotFoundError("Course not found.") from err if not any(tab.type == 'discussion' and tab.is_enabled(course, user) for tab in course.tabs): raise DiscussionDisabledError("Discussion is disabled for the course.") return course @@ -143,10 +140,10 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None): if requester_group_id is not None and cc_thread["group_id"] != requester_group_id: raise ThreadNotFoundError("Thread not found.") return cc_thread, context - except CommentClientRequestError: + except CommentClientRequestError as err: # params are validated at a higher level, so the only possible request # error is if the thread doesn't exist - raise ThreadNotFoundError("Thread not found.") # lint-amnesty, pylint: disable=raise-missing-from + raise ThreadNotFoundError("Thread not found.") from err def _get_comment_and_context(request, comment_id): @@ -161,8 +158,8 @@ def _get_comment_and_context(request, comment_id): cc_comment = Comment(id=comment_id).retrieve() _, context = _get_thread_and_context(request, cc_comment["thread_id"]) return cc_comment, context - except CommentClientRequestError: - raise CommentNotFoundError("Comment not found.") # lint-amnesty, pylint: disable=raise-missing-from + except CommentClientRequestError as err: + raise CommentNotFoundError("Comment not found.") from err def _is_user_author_or_privileged(cc_content, context): @@ -833,6 +830,8 @@ def _do_extra_actions(api_content, cc_content, request_fields, actions_form, con _handle_voted_field(form_value, cc_content, api_content, request, context) elif field == "read": _handle_read_field(api_content, form_value, context["cc_requester"], cc_content) + elif field == "pinned": + _handle_pinned_field(form_value, cc_content, context["cc_requester"]) else: raise ValidationError({field: ["Invalid Key"]}) @@ -879,6 +878,22 @@ def _handle_read_field(api_content, form_value, user, cc_content): api_content["unread_comment_count"] = 0 +def _handle_pinned_field(pin_thread: bool, cc_content: Thread, user: User): + """ + Pins or unpins a thread + + Arguments: + + pin_thread (bool): Value of field from API + cc_content (Thread): The thread on which to operate + user (User): The user performing the action + """ + if pin_thread: + cc_content.pin(user, cc_content.id) + else: + cc_content.un_pin(user, cc_content.id) + + def create_thread(request, thread_data): """ Create a thread. @@ -902,8 +917,8 @@ def create_thread(request, thread_data): try: course_key = CourseKey.from_string(course_id) course = _get_course(course_key, user) - except InvalidKeyError: - raise ValidationError({"course_id": ["Invalid value."]}) # lint-amnesty, pylint: disable=raise-missing-from + except InvalidKeyError as err: + raise ValidationError({"course_id": ["Invalid value."]}) from err if not discussion_open_for_user(course, user): raise DiscussionBlackOutException @@ -1143,8 +1158,8 @@ def get_response_comments(request, comment_id, page, page_size, requested_fields num_pages = (comments_count + page_size - 1) // page_size if comments_count else 1 paginator = DiscussionAPIPagination(request, page, num_pages, comments_count) return paginator.get_paginated_response(results) - except CommentClientRequestError: - raise CommentNotFoundError("Comment not found") # lint-amnesty, pylint: disable=raise-missing-from + except CommentClientRequestError as err: + raise CommentNotFoundError("Comment not found") from err def delete_thread(request, thread_id): diff --git a/lms/djangoapps/discussion/rest_api/forms.py b/lms/djangoapps/discussion/rest_api/forms.py index 3efbb56277..486f287fac 100644 --- a/lms/djangoapps/discussion/rest_api/forms.py +++ b/lms/djangoapps/discussion/rest_api/forms.py @@ -110,6 +110,7 @@ class ThreadActionsForm(Form): voted = BooleanField(required=False) abuse_flagged = BooleanField(required=False) read = BooleanField(required=False) + pinned = BooleanField(required=False) class CommentListGetForm(_PaginationForm): diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index e3c32689e2..bca338700c 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -1,7 +1,7 @@ """ Discussion API permission logic """ - +from typing import Dict, Set, Union from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread @@ -54,46 +54,61 @@ def get_initializable_comment_fields(context): return ret -def get_editable_fields(cc_content, context): +def _filter_fields(editable_fields: Dict[str, bool]) -> Set[str]: + """ + Helper function that returns only the keys marked as True. + Args: + editable_fields (Dict[str, bool]): A mapping of strings to a bool value + that indicates whether they should be in the output set + + Returns: + Set[str] a set of fields that have a true value. + """ + return {field for field, is_editable in editable_fields.items() if is_editable} + + +def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Set[str]: """ Return the set of fields that the requester can edit on the given content """ - # 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"]: - ret |= {"read"} - return ret - if cc_content["type"] == "comment" and context["thread"]["closed"]: - return ret + is_thread = cc_content["type"] == "thread" + is_comment = cc_content["type"] == "comment" + is_privileged = context["is_requester_privileged"] + # True if we're dealing with a closed thread or a comment in a closed thread + is_thread_closed = cc_content["closed"] if is_thread else context["thread"]["closed"] - # Shared fields - ret |= {"voted"} - if _is_author_or_privileged(cc_content, context): - ret |= {"raw_body"} + # Map each field to the condition in which it's editable. + editable_fields = { + "abuse_flagged": True, + "closed": is_thread and is_privileged, + "pinned": is_thread and is_privileged, + "read": is_thread, + } - # Thread fields - if cc_content["type"] == "thread": - ret |= {"following", "read"} - if _is_author_or_privileged(cc_content, context): - ret |= {"topic_id", "type", "title"} - if context["is_requester_privileged"] and context["discussion_division_enabled"]: - ret |= {"group_id"} + if is_thread_closed: + # Return only editable fields + return _filter_fields(editable_fields) - # 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 + is_author = _is_author(cc_content, context) + editable_fields.update({ + "voted": True, + "raw_body": is_privileged or is_author, + "following": is_thread, + "topic_id": is_thread and (is_author or is_privileged), + "type": is_thread and (is_author or is_privileged), + "title": is_thread and (is_author or is_privileged), + "group_id": is_thread and is_privileged and context["discussion_division_enabled"], + "endorsed": ( + is_comment and + (is_privileged or + (_is_author(context["thread"], context) and context["thread"]["thread_type"] == "question")) + ) + }) + # Return only editable fields + return _filter_fields(editable_fields) def can_delete(cc_content, context): diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 515938b14a..c0da0bb447 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -7,6 +7,8 @@ from urllib.parse import urlencode, urlunparse from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.urls import reverse +from django.utils.html import strip_tags +from django.utils.text import Truncator from rest_framework import serializers from common.djangoapps.student.models import get_user_by_username_or_email @@ -21,6 +23,7 @@ from lms.djangoapps.discussion.django_comment_client.utils import ( from lms.djangoapps.discussion.rest_api.permissions import ( NON_UPDATABLE_COMMENT_FIELDS, NON_UPDATABLE_THREAD_FIELDS, + can_delete, get_editable_fields, ) from lms.djangoapps.discussion.rest_api.render import render_body @@ -120,11 +123,13 @@ class _ContentSerializer(serializers.Serializer): voted = serializers.SerializerMethodField() vote_count = serializers.SerializerMethodField() editable_fields = serializers.SerializerMethodField() + can_delete = serializers.SerializerMethodField() non_updatable_fields = set() def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + self._rendered_body = None for field in self.non_updatable_fields: setattr(self, f"validate_{field}", self._validate_non_updatable) @@ -183,7 +188,9 @@ class _ContentSerializer(serializers.Serializer): """ Returns the rendered body content. """ - return render_body(obj["body"]) + if self._rendered_body is None: + self._rendered_body = render_body(obj["body"]) + return self._rendered_body def get_abuse_flagged(self, obj): """ @@ -211,6 +218,12 @@ class _ContentSerializer(serializers.Serializer): """ return sorted(get_editable_fields(obj, self.context)) + def get_can_delete(self, obj): + """ + Returns if the current user can delete this thread/comment. + """ + return can_delete(obj, self.context) + class ThreadSerializer(_ContentSerializer): """ @@ -228,10 +241,11 @@ class ThreadSerializer(_ContentSerializer): source="thread_type", choices=[(val, val) for val in ["discussion", "question"]] ) + preview_body = serializers.SerializerMethodField() abuse_flagged_count = serializers.SerializerMethodField(required=False) title = serializers.CharField(validators=[validate_not_blank]) - pinned = serializers.SerializerMethodField(read_only=True) - closed = serializers.BooleanField(read_only=True) + pinned = serializers.SerializerMethodField() + closed = serializers.BooleanField(required=False) following = serializers.SerializerMethodField() comment_count = serializers.SerializerMethodField(read_only=True) unread_comment_count = serializers.SerializerMethodField(read_only=True) @@ -325,6 +339,13 @@ class ThreadSerializer(_ContentSerializer): return obj["unread_comments_count"] + 1 return obj["unread_comments_count"] + def get_preview_body(self, obj): + """ + Returns a cleaned and truncated version of the thread's body to display in a + preview capacity. + """ + return Truncator(strip_tags(self.get_rendered_body(obj))).words(10, ) + def create(self, validated_data): thread = Thread(user_id=self.context["cc_requester"]["id"], **validated_data) thread.save() diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 615206da29..dded8a94f1 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -734,6 +734,7 @@ class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMix "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", "abuse_flagged_count": None, + "can_delete": False, }), self.expected_thread_data({ "id": "test_thread_id_1", @@ -744,6 +745,7 @@ class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMix "type": "question", "title": "Another Test Title", "raw_body": "More content", + "preview_body": "More content", "rendered_body": "

More content

", "vote_count": 9, "comment_count": 19, @@ -758,6 +760,7 @@ class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMix ), "editable_fields": ["abuse_flagged", "following", "read", "voted"], "abuse_flagged_count": None, + "can_delete": False, }), ] @@ -1353,6 +1356,7 @@ class GetCommentListTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModu "editable_fields": ["abuse_flagged", "voted"], "child_count": 0, "children": [], + "can_delete": False, }, { "id": "test_comment_2", @@ -1375,6 +1379,7 @@ class GetCommentListTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModu "editable_fields": ["abuse_flagged", "voted"], "child_count": 0, "children": [], + "can_delete": False, }, ] actual_comments = self.get_comment_list( @@ -1688,8 +1693,12 @@ class CreateThreadTest( "course_id": str(self.course.id), "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_id", "read": True, + "editable_fields": [ + "abuse_flagged", "closed", "following", "pinned", "raw_body", "read", "title", "topic_id", "type", + "voted" + ], }) - self.assertEqual(actual, expected) + assert actual == expected self.assertEqual( httpretty.last_request().parsed_body, # lint-amnesty, pylint: disable=no-member { @@ -1970,6 +1979,7 @@ class CreateCommentTest( "children": [], "editable_fields": ["abuse_flagged", "raw_body", "voted"], "child_count": 0, + "can_delete": True, } assert actual == expected expected_url = ( @@ -2051,6 +2061,7 @@ class CreateCommentTest( "children": [], "editable_fields": ["abuse_flagged", "endorsed", "raw_body", "voted"], "child_count": 0, + "can_delete": True, } assert actual == expected expected_url = ( @@ -2305,6 +2316,7 @@ class UpdateThreadTest( assert actual == self.expected_thread_data({ 'raw_body': 'Edited body', 'rendered_body': '

Edited body

', + 'preview_body': 'Edited body', 'topic_id': 'original_topic', 'read': True, 'title': 'Original Title' @@ -2684,6 +2696,7 @@ class UpdateCommentTest( "children": [], "editable_fields": ["abuse_flagged", "raw_body", "voted"], "child_count": 0, + "can_delete": True, } assert actual == expected assert httpretty.last_request().parsed_body == { # lint-amnesty, pylint: disable=no-member @@ -3322,6 +3335,7 @@ class RetrieveThreadTest( self.register_thread() self.request.user = non_author_user assert get_thread(self.request, self.thread_id) == self.expected_thread_data({ + 'can_delete': False, 'editable_fields': ['abuse_flagged', 'following', 'read', 'voted'], 'unread_comment_count': 1 }) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py index 74d20eb900..57c419c360 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py @@ -46,6 +46,8 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): expected = { "abuse_flagged", "course_id", "following", "raw_body", "read", "title", "topic_id", "type", "voted" } + if is_privileged: + expected |= {"closed", "pinned"} if is_privileged and is_cohorted: expected |= {"group_id"} assert actual == expected @@ -81,6 +83,8 @@ class GetEditableFieldsTest(ModuleStoreTestCase): ) actual = get_editable_fields(thread, context) expected = {"abuse_flagged", "following", "read", "voted"} + if is_privileged: + expected |= {"closed", "pinned"} if is_author or is_privileged: expected |= {"topic_id", "type", "title", "raw_body"} if is_privileged and is_cohorted: diff --git a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py index 353aee0e5f..888cac0f26 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py @@ -180,6 +180,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, SharedModuleStoreTe }) expected = self.expected_thread_data({ "author": self.author.username, + "can_delete": False, "vote_count": 4, "comment_count": 6, "unread_comment_count": 3, @@ -312,6 +313,7 @@ class CommentSerializerTest(SerializerTestMixin, SharedModuleStoreTestCase): "children": [], "editable_fields": ["abuse_flagged", "voted"], "child_count": 0, + "can_delete": False, } assert self.serialize(comment) == expected diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index e4a46a1a4d..61103af01c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -545,6 +545,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro "updated_at": "2015-04-28T11:11:11Z", "vote_count": 4, "comment_count": 6, + "can_delete": False, "unread_comment_count": 3, "voted": True, "author": self.author.username, @@ -849,7 +850,7 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "topic_id": "test_topic", "type": "discussion", "title": "Test Title", - "raw_body": "Test body", + "raw_body": "# Test \n This is a very long body that will be truncated for the preview.", } response = self.client.post( self.url, @@ -858,13 +859,18 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) assert response.status_code == 200 response_data = json.loads(response.content.decode('utf-8')) - assert response_data == self.expected_thread_data({'read': True}) + assert response_data == self.expected_thread_data({ + "read": True, + "raw_body": "# Test \n This is a very long body that will be truncated for the preview.", + "preview_body": "Test This is a very long body that will be…", + "rendered_body": "

Test

\n

This is a very long body that will be truncated for the preview.

", + }) assert httpretty.last_request().parsed_body == { # lint-amnesty, pylint: disable=no-member 'course_id': [str(self.course.id)], 'commentable_id': ['test_topic'], 'thread_type': ['discussion'], 'title': ['Test Title'], - 'body': ['Test body'], + "body": ["# Test \n This is a very long body that will be truncated for the preview."], 'user_id': [str(self.user.id)] } @@ -914,6 +920,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest assert response_data == self.expected_thread_data({ 'raw_body': 'Edited body', 'rendered_body': '

Edited body

', + 'preview_body': 'Edited body', 'editable_fields': ['abuse_flagged', 'following', 'raw_body', 'read', 'title', 'topic_id', 'type', 'voted'], 'created_at': 'Test Created Date', 'updated_at': 'Test Updated Date', @@ -1017,6 +1024,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest assert response_data == self.expected_thread_data({ 'author': str(thread_owner_user.username), 'comment_count': 1, + 'can_delete': False, 'read': True, 'editable_fields': ['abuse_flagged', 'following', 'read', 'voted'], 'response_count': 2 @@ -1119,6 +1127,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr "children": [], "editable_fields": ["abuse_flagged", "voted"], "child_count": 0, + "can_delete": True, } response_data.update(overrides or {}) return response_data @@ -1149,6 +1158,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr "voted": True, "vote_count": 4, "raw_body": "Test body", + "can_delete": False, "rendered_body": "

Test body

", "created_at": "2015-05-11T00:00:00Z", "updated_at": "2015-05-11T11:11:11Z", @@ -1312,8 +1322,8 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr self.register_get_thread_response(thread) response = self.client.get(self.url, {"thread_id": self.thread_id}) expected_comments = [ - self.expected_response_comment(overrides={"id": "test_response_1", "child_count": 2}), - self.expected_response_comment(overrides={"id": "test_response_2", "child_count": 3}), + self.expected_response_comment(overrides={"id": "test_response_1", "child_count": 2, "can_delete": False}), + self.expected_response_comment(overrides={"id": "test_response_2", "child_count": 3, "can_delete": False}), ] self.assert_response_correct( response, @@ -1507,6 +1517,7 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "children": [], "editable_fields": ["abuse_flagged", "raw_body", "voted"], "child_count": 0, + "can_delete": True, } response = self.client.post( self.url, @@ -1592,6 +1603,7 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes "children": [], "editable_fields": [], "child_count": 0, + "can_delete": True, } response_data.update(overrides or {}) return response_data @@ -1776,6 +1788,7 @@ class CommentViewSetRetrieveTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase "abuse_flagged_any_user": None, "editable_fields": ["abuse_flagged", "raw_body", "voted"], "child_count": 0, + "can_delete": True, } response = self.client.get(self.url) diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index c05e3231e3..b062f3c72c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -387,6 +387,7 @@ class CommentsServiceMockMixin: "updated_at": "1970-01-01T00:00:00Z", "raw_body": "Test body", "rendered_body": "

Test body

", + "preview_body": "Test body", "abuse_flagged": False, "abuse_flagged_count": None, "voted": False, @@ -399,6 +400,7 @@ class CommentsServiceMockMixin: "title": "Test Title", "pinned": False, "closed": False, + "can_delete": True, "following": False, "comment_count": 1, "unread_comment_count": 0,