Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

feat: Add support for pinning and closing threads, adds a preview and a delete check field (#28809)

Browse Source 
Adds support for pinning and closing threads to the current REST APIs for discusions.
Adds a preview_body field to the thread response that contains a truncated version of the raw body with tags removed so it can be shown in previews.
Adds a field called "can_delete" to threads and comments that signals if the current user is able to delete that resource.
This commit is contained in:
Kshitij Sobti
2021-10-01 14:57:12 +00:00
committed by GitHub
parent bf025a9b0c
commit dd6e908635
9 changed files with 142 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
lms/djangoapps/discussion/rest_api/api.py
View File

@@ -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):

1
lms/djangoapps/discussion/rest_api/forms.py
View File

@@ -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):

79
lms/djangoapps/discussion/rest_api/permissions.py
View File

@@ -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):

27
lms/djangoapps/discussion/rest_api/serializers.py
View File

@@ -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()

16
lms/djangoapps/discussion/rest_api/tests/test_api.py
View File

@@ -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": "<p>More content</p>",
"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': '<p>Edited body</p>',
'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
})

4
lms/djangoapps/discussion/rest_api/tests/test_permissions.py
View File

@@ -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:

2
lms/djangoapps/discussion/rest_api/tests/test_serializers.py
View File

@@ -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

23
lms/djangoapps/discussion/rest_api/tests/test_views.py
View File

@@ -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": "<h1>Test</h1>\n<p>This is a very long body that will be truncated for the preview.</p>",
})
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': '<p>Edited body</p>',
'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": "<p>Test body</p>",
"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)

2
lms/djangoapps/discussion/rest_api/tests/utils.py
View File

@@ -387,6 +387,7 @@ class CommentsServiceMockMixin:
"updated_at": "1970-01-01T00:00:00Z",
"raw_body": "Test body",
"rendered_body": "<p>Test body</p>",
"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,