From 72d060cbeb1a02c740658ac086e05383e110b877 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Thu, 22 Aug 2024 12:34:30 -0400 Subject: [PATCH 01/12] feat: update XBlock to 5.1.0 (#35325) --- requirements/constraints.txt | 4 ---- requirements/edx/base.txt | 3 +-- requirements/edx/development.txt | 3 +-- requirements/edx/doc.txt | 3 +-- requirements/edx/testing.txt | 3 +-- 5 files changed, 4 insertions(+), 12 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index aa45d9944d5a..146d8605101a 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -142,7 +142,3 @@ django-storages<1.14.4 # We are pinning this until after all the smaller migrations get handled and then we can migrate this all at once. # Ticket to unpin: https://github.com/edx/edx-arch-experiments/issues/760 social-auth-app-django<=5.4.1 - -# Xblock==5.0.0 changed how entrypoints were loaded, breaking a workaround for overriding blocks. -# See ticket: https://github.com/openedx/XBlock/issues/777 -xblock[django]==4.0.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 873446da1353..48b31cad0356 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -1258,9 +1258,8 @@ webob==1.8.8 # xblock wrapt==1.16.0 # via -r requirements/edx/paver.txt -xblock[django]==4.0.1 +xblock[django]==5.1.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in # acid-xblock # crowdsourcehinter-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 4951b16608b6..a4825209b60d 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -2231,9 +2231,8 @@ wrapt==1.16.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # astroid -xblock[django]==4.0.1 +xblock[django]==5.1.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # acid-xblock diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 8c9c7bbbb379..d4bc6c14d37d 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1549,9 +1549,8 @@ webob==1.8.8 # xblock wrapt==1.16.0 # via -r requirements/edx/base.txt -xblock[django]==4.0.1 +xblock[django]==5.1.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # acid-xblock # crowdsourcehinter-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 87c98537d3a7..6527b45b56bf 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1646,9 +1646,8 @@ wrapt==1.16.0 # via # -r requirements/edx/base.txt # astroid -xblock[django]==4.0.1 +xblock[django]==5.1.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # acid-xblock # crowdsourcehinter-xblock From 6071992281437ae0eaa57c66755a74290ab2953f Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Thu, 22 Aug 2024 14:56:51 -0400 Subject: [PATCH 02/12] feat: lint this file (#35348) I'm about to make a bunch of changes to this file, and before I do I'm saving it and letting the linter reformatted to our current code style standards, so that code reviewers won't have to read a mix of lint and code changes. FIXES: APER-3554 --- openedx/core/djangoapps/user_api/views.py | 41 ++++++++++++----------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index b4fcc68db649..d52493556a19 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -1,6 +1,5 @@ """HTTP end-points for the User API. """ - from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.http import HttpResponse from django.utils.decorators import method_decorator @@ -16,21 +15,22 @@ from rest_framework.views import APIView from openedx.core.djangoapps.django_comment_common.models import Role -from openedx.core.lib.api.view_utils import require_post_params from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangoapps.user_api.preferences.api import get_country_time_zones, update_email_opt_in from openedx.core.djangoapps.user_api.serializers import ( CountryTimeZoneSerializer, UserPreferenceSerializer, - UserSerializer + UserSerializer, ) from openedx.core.lib.api.permissions import ApiKeyHeaderPermission +from openedx.core.lib.api.view_utils import require_post_params class UserViewSet(viewsets.ReadOnlyModelViewSet): """ DRF class for interacting with the User ORM object """ + permission_classes = (ApiKeyHeaderPermission,) queryset = User.objects.all().prefetch_related("preferences").select_related("profile") serializer_class = UserSerializer @@ -42,6 +42,7 @@ class ForumRoleUsersListView(generics.ListAPIView): """ Forum roles are represented by a list of user dicts """ + permission_classes = (ApiKeyHeaderPermission,) serializer_class = UserSerializer paginate_by = 10 @@ -51,10 +52,10 @@ def get_queryset(self): """ Return a list of users with the specified role/course pair """ - name = self.kwargs['name'] - course_id_string = self.request.query_params.get('course_id') + name = self.kwargs["name"] + course_id_string = self.request.query_params.get("course_id") if not course_id_string: - raise ParseError('course_id must be specified') + raise ParseError("course_id must be specified") course_id = CourseKey.from_string(course_id_string) role = Role.objects.get_or_create(course_id=course_id, name=name)[0] users = role.users.prefetch_related("preferences").select_related("profile").all() @@ -65,6 +66,7 @@ class UserPreferenceViewSet(viewsets.ReadOnlyModelViewSet): """ DRF class for interacting with the UserPreference ORM """ + permission_classes = (ApiKeyHeaderPermission,) queryset = UserPreference.objects.all() filter_backends = (DjangoFilterBackend,) @@ -78,26 +80,30 @@ class PreferenceUsersListView(generics.ListAPIView): """ DRF class for listing a user's preferences """ + permission_classes = (ApiKeyHeaderPermission,) serializer_class = UserSerializer paginate_by = 10 paginate_by_param = "page_size" def get_queryset(self): - return User.objects.filter( - preferences__key=self.kwargs["pref_key"] - ).prefetch_related("preferences").select_related("profile") + return ( + User.objects.filter(preferences__key=self.kwargs["pref_key"]) + .prefetch_related("preferences") + .select_related("profile") + ) class UpdateEmailOptInPreference(APIView): - """View for updating the email opt in preference. """ + """View for updating the email opt in preference.""" + authentication_classes = (SessionAuthenticationAllowInactiveUser,) permission_classes = (IsAuthenticated,) @method_decorator(require_post_params(["course_id", "email_opt_in"])) @method_decorator(ensure_csrf_cookie) def post(self, request): - """ Post function for updating the email opt in preference. + """Post function for updating the email opt in preference. Allows the modification or creation of the email opt in preference at an organizational level. @@ -111,17 +117,13 @@ def post(self, request): assume False. """ - course_id = request.data['course_id'] + course_id = request.data["course_id"] try: org = locator.CourseLocator.from_string(course_id).org except InvalidKeyError: - return HttpResponse( - status=400, - content=f"No course '{course_id}' found", - content_type="text/plain" - ) + return HttpResponse(status=400, content=f"No course '{course_id}' found", content_type="text/plain") # Only check for true. All other values are False. - email_opt_in = request.data['email_opt_in'].lower() == 'true' + email_opt_in = request.data["email_opt_in"].lower() == "true" update_email_opt_in(request.user, org, email_opt_in) return HttpResponse(status=status.HTTP_200_OK) @@ -152,9 +154,10 @@ class CountryTimeZoneListView(generics.ListAPIView): * time_zone: The name of the time zone. * description: The display version of the time zone """ + serializer_class = CountryTimeZoneSerializer paginator = None def get_queryset(self): - country_code = self.request.GET.get('country_code', None) + country_code = self.request.GET.get("country_code", None) return get_country_time_zones(country_code) From 36a3b0ba8178aa479464b9eccfb9f9b0e5e0240e Mon Sep 17 00:00:00 2001 From: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com> Date: Thu, 22 Aug 2024 15:49:21 -0400 Subject: [PATCH 03/12] Revert "fix: update course discussion config before course load (#35219)" (#35349) This reverts commit 5c0942481ce292a90f86259bd223d66e7ceffe9f. --- cms/djangoapps/contentstore/views/course.py | 5 ----- cms/djangoapps/contentstore/views/tests/test_course_index.py | 4 ++-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4647e4fdcca7..9f6cfb7c430e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -58,7 +58,6 @@ from common.djangoapps.util.string_utils import _has_non_ascii_characters from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements -from openedx.core.djangoapps.discussions.tasks import update_discussions_settings_from_course from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangolib.js_utils import dump_js_escaped_json @@ -303,10 +302,6 @@ def course_handler(request, course_key_string=None): else: return HttpResponseBadRequest() elif request.method == 'GET': # assume html - # Update course discussion settings, sometimes the course discussion settings are not updated - # when the course is created, so we need to update them here. - course_key = CourseKey.from_string(course_key_string) - update_discussions_settings_from_course(course_key) if course_key_string is None: return redirect(reverse('home')) else: diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 30e02214a1a8..c3dcfe5305b7 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -717,8 +717,8 @@ def test_number_of_calls_to_db(self): """ Test to check number of queries made to mysql and mongo """ - with self.assertNumQueries(32, table_ignorelist=WAFFLE_TABLES): - with check_mongo_calls(5): + with self.assertNumQueries(29, table_ignorelist=WAFFLE_TABLES): + with check_mongo_calls(3): self.client.get_html(reverse_course_url('course_handler', self.course.id)) From 5fbcc794cfb49d30e9f4eca6536234817b57b7f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 23 Aug 2024 02:03:31 -0300 Subject: [PATCH 04/12] feat: add collections app from openedx-learning (#35312) --- cms/envs/common.py | 1 + lms/envs/common.py | 1 + requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 7 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index be837c518981..45a8e97f3e51 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1880,6 +1880,7 @@ 'openedx_events', # Learning Core Apps, used by v2 content libraries (content_libraries app) + "openedx_learning.apps.authoring.collections", "openedx_learning.apps.authoring.components", "openedx_learning.apps.authoring.contents", "openedx_learning.apps.authoring.publishing", diff --git a/lms/envs/common.py b/lms/envs/common.py index 18d07afd7161..04a1753838ed 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3394,6 +3394,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx_events', # Learning Core Apps, used by v2 content libraries (content_libraries app) + "openedx_learning.apps.authoring.collections", "openedx_learning.apps.authoring.components", "openedx_learning.apps.authoring.contents", "openedx_learning.apps.authoring.publishing", diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 146d8605101a..74263b5f7141 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -93,7 +93,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.10.1 +openedx-learning==0.11.1 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 48b31cad0356..27de7847f284 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -823,7 +823,7 @@ openedx-filters==1.9.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning==0.10.1 +openedx-learning==0.11.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index a4825209b60d..0bdc8144ee71 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1372,7 +1372,7 @@ openedx-filters==1.9.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning==0.10.1 +openedx-learning==0.11.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d4bc6c14d37d..91b30d81df6d 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -982,7 +982,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.10.1 +openedx-learning==0.11.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 6527b45b56bf..2092c9354834 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1033,7 +1033,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.10.1 +openedx-learning==0.11.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 71f410eb4072257df8fdd99c0200c27bdee0d112 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Sun, 25 Aug 2024 23:24:05 -0700 Subject: [PATCH 05/12] feat: added snowflake events for email digest unsubscribe (#35329) --- .../core/djangoapps/notifications/email/utils.py | 2 ++ openedx/core/djangoapps/notifications/events.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/openedx/core/djangoapps/notifications/email/utils.py b/openedx/core/djangoapps/notifications/email/utils.py index da288750bb7d..e36c435e8ac0 100644 --- a/openedx/core/djangoapps/notifications/email/utils.py +++ b/openedx/core/djangoapps/notifications/email/utils.py @@ -19,6 +19,7 @@ ) from openedx.core.djangoapps.notifications.config.waffle import ENABLE_EMAIL_NOTIFICATIONS from openedx.core.djangoapps.notifications.email_notifications import EmailCadence +from openedx.core.djangoapps.notifications.events import notification_preference_unsubscribe_event from openedx.core.djangoapps.notifications.models import ( CourseNotificationPreference, get_course_notification_preference_config_version @@ -395,3 +396,4 @@ def get_updated_preference(pref): if pref_value else EmailCadence.NEVER type_prefs['email_cadence'] = cadence_value preference.save() + notification_preference_unsubscribe_event(user) diff --git a/openedx/core/djangoapps/notifications/events.py b/openedx/core/djangoapps/notifications/events.py index fb50e134941b..91b12075a8a1 100644 --- a/openedx/core/djangoapps/notifications/events.py +++ b/openedx/core/djangoapps/notifications/events.py @@ -10,6 +10,7 @@ NOTIFICATION_APP_ALL_READ = 'edx.notifications.app_all_read' NOTIFICATION_PREFERENCES_UPDATED = 'edx.notifications.preferences.updated' NOTIFICATION_TRAY_OPENED = 'edx.notifications.tray_opened' +NOTIFICATION_PREFERENCE_UNSUBSCRIBE = 'edx.notifications.preferences.one_click_unsubscribe' def get_user_forums_roles(user, course_id): @@ -155,3 +156,17 @@ def notification_tray_opened_event(user, unseen_notifications_count): 'unseen_notifications_count': unseen_notifications_count, } ) + + +def notification_preference_unsubscribe_event(user): + """ + Emits an event when user clicks on one-click-unsubscribe url + """ + event_data = { + 'user_id': user.id, + 'username': user.username, + 'event_type': 'email_digest_unsubscribe' + } + with tracker.get_tracker().context(NOTIFICATION_PREFERENCE_UNSUBSCRIBE, event_data): + tracker.emit(NOTIFICATION_PREFERENCE_UNSUBSCRIBE, event_data) + segment.track(user.id, NOTIFICATION_PREFERENCE_UNSUBSCRIBE, event_data) From 66f3a0803c99cdc14ca809d20cd98a0ee4140783 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Mon, 26 Aug 2024 13:13:47 +0500 Subject: [PATCH 06/12] feat: added email content in misc notifications (#35341) --- .../rest_api/discussions_notifications.py | 65 ++++++++++++-- lms/djangoapps/discussion/rest_api/tasks.py | 4 +- .../tests/test_discussions_notifications.py | 84 ++++++++++++++++++- .../discussion/rest_api/tests/test_tasks.py | 67 +++++++++++++-- .../discussion/rest_api/tests/utils.py | 3 +- lms/djangoapps/discussion/signals/handlers.py | 3 +- 6 files changed, 209 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py index 21b1e27fcdf8..96e392c35d2a 100644 --- a/lms/djangoapps/discussion/rest_api/discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/discussions_notifications.py @@ -3,7 +3,10 @@ """ import re +from bs4 import BeautifulSoup from django.conf import settings +from django.utils.text import Truncator + from lms.djangoapps.discussion.django_comment_client.permissions import get_team from openedx_events.learning.data import UserNotificationData, CourseNotificationData from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED @@ -27,13 +30,24 @@ class DiscussionNotificationSender: Class to send notifications to users who are subscribed to the thread. """ - def __init__(self, thread, course, creator, parent_id=None): + def __init__(self, thread, course, creator, parent_id=None, comment_id=None): self.thread = thread self.course = course self.creator = creator self.parent_id = parent_id + self.comment_id = comment_id self.parent_response = None + self.comment = None self._get_parent_response() + self._get_comment() + + def _get_comment(self): + """ + Get comment object + """ + if not self.comment_id: + return + self.comment = Comment(id=self.comment_id).retrieve() def _send_notification(self, user_ids, notification_type, extra_context=None): """ @@ -99,7 +113,10 @@ def send_new_response_notification(self): there is a response to the main thread. """ if not self.parent_id and self.creator.id != int(self.thread.user_id): - self._send_notification([self.thread.user_id], "new_response") + context = { + 'email_content': clean_thread_html_body(self.comment.body), + } + self._send_notification([self.thread.user_id], "new_response", extra_context=context) def _response_and_thread_has_same_creator(self) -> bool: """ @@ -136,6 +153,7 @@ def send_new_comment_notification(self): context = { "author_name": str(author_name), "author_pronoun": str(author_pronoun), + "email_content": clean_thread_html_body(self.comment.body), } self._send_notification([self.thread.user_id], "new_comment", extra_context=context) @@ -149,7 +167,14 @@ def send_new_comment_on_response_notification(self): self.creator.id != int(self.parent_response.user_id) and not self._response_and_thread_has_same_creator() ): - self._send_notification([self.parent_response.user_id], "new_comment_on_response") + context = { + "email_content": clean_thread_html_body(self.comment.body), + } + self._send_notification( + [self.parent_response.user_id], + "new_comment_on_response", + extra_context=context + ) def _check_if_subscriber_is_not_thread_or_content_creator(self, subscriber_id) -> bool: """ @@ -190,7 +215,12 @@ def send_response_on_followed_post_notification(self): # Remove duplicate users from the list of users to send notification users = list(set(users)) if not self.parent_id: - self._send_notification(users, "response_on_followed_post") + self._send_notification( + users, + "response_on_followed_post", + extra_context={ + "email_content": clean_thread_html_body(self.comment.body), + }) else: author_name = f"{self.parent_response.username}'s" # use 'their' if comment author is also response author. @@ -206,6 +236,7 @@ def send_response_on_followed_post_notification(self): extra_context={ "author_name": str(author_name), "author_pronoun": str(author_pronoun), + "email_content": clean_thread_html_body(self.comment.body), } ) @@ -289,7 +320,8 @@ def send_new_thread_created_notification(self): ] context = { 'username': self.creator.username, - 'post_title': self.thread.title + 'post_title': self.thread.title, + "email_content": clean_thread_html_body(self.thread.body), } self._send_course_wide_notification(notification_type, audience_filters, context) @@ -339,3 +371,26 @@ def is_discussion_cohorted(course_key_str): def remove_html_tags(text): clean = re.compile('<.*?>') return re.sub(clean, '', text) + + +def clean_thread_html_body(html_body): + """ + Get post body with tags removed and limited to 500 characters + """ + html_body = BeautifulSoup(Truncator(html_body).chars(500, html=True), 'html.parser') + + tags_to_remove = [ + "a", "link", # Link Tags + "img", "picture", "source", # Image Tags + "video", "track", # Video Tags + "audio", # Audio Tags + "embed", "object", "iframe", # Embedded Content + "script" + ] + + # Remove the specified tags while keeping their content + for tag in tags_to_remove: + for match in html_body.find_all(tag): + match.unwrap() + + return str(html_body) diff --git a/lms/djangoapps/discussion/rest_api/tasks.py b/lms/djangoapps/discussion/rest_api/tasks.py index 45bf41fe905f..a87fafd4ca54 100644 --- a/lms/djangoapps/discussion/rest_api/tasks.py +++ b/lms/djangoapps/discussion/rest_api/tasks.py @@ -33,7 +33,7 @@ def send_thread_created_notification(thread_id, course_key_str, user_id): @shared_task @set_code_owner_attribute -def send_response_notifications(thread_id, course_key_str, user_id, parent_id=None): +def send_response_notifications(thread_id, course_key_str, user_id, comment_id, parent_id=None): """ Send notifications to users who are subscribed to the thread. """ @@ -43,7 +43,7 @@ def send_response_notifications(thread_id, course_key_str, user_id, parent_id=No thread = Thread(id=thread_id).retrieve() user = User.objects.get(id=user_id) course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) - notification_sender = DiscussionNotificationSender(thread, course, user, parent_id) + notification_sender = DiscussionNotificationSender(thread, course, user, parent_id, comment_id) notification_sender.send_new_comment_notification() notification_sender.send_new_response_notification() notification_sender.send_new_comment_on_response_notification() diff --git a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py index b4d3f3d18a0d..f1a71fd1239e 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py @@ -1,13 +1,14 @@ """ Unit tests for the DiscussionNotificationSender class """ - +import re import unittest from unittest.mock import MagicMock, patch import pytest -from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender +from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender, \ + clean_thread_html_body @patch('lms.djangoapps.discussion.rest_api.discussions_notifications.DiscussionNotificationSender' @@ -88,3 +89,82 @@ def test_send_reported_content_notification_for_thread(self, mock_send_notificat self.notification_sender.send_reported_content_notification() self._assert_send_notification_called_with(mock_send_notification, 'thread') + + +class TestCleanThreadHtmlBody(unittest.TestCase): + """ + Tests for the clean_thread_html_body function + """ + + def test_html_tags_removal(self): + """ + Test that the clean_thread_html_body function removes unwanted HTML tags + """ + html_body = """ +

This is a link to a page.

+

Here is an image: image

+

Embedded video:

+

Script test:

+

Some other content that should remain.

+ """ + expected_output = ("

This is a link to a page.

" + "

Here is an image:

" + "

Embedded video:

" + "

Script test: alert('hello');

" + "

Some other content that should remain.

") + + result = clean_thread_html_body(html_body) + + def normalize_html(text): + """ + Normalize the output by removing extra whitespace, newlines, and spaces between tags + """ + text = re.sub(r'\s+', ' ', text).strip() # Replace any sequence of whitespace with a single space + text = re.sub(r'>\s+<', '><', text) # Remove spaces between HTML tags + return text + + normalized_result = normalize_html(result) + normalized_expected_output = normalize_html(expected_output) + + self.assertEqual(normalized_result, normalized_expected_output) + + def test_truncate_html_body(self): + """ + Test that the clean_thread_html_body function truncates the HTML body to 500 characters + """ + html_body = """ +

This is a long text that should be truncated to 500 characters.

+ """ * 20 # Repeat to exceed 500 characters + + result = clean_thread_html_body(html_body) + self.assertGreaterEqual(500, len(result)) + + def test_no_tags_to_remove(self): + """ + Test that the clean_thread_html_body function does not remove any tags if there are no unwanted tags + """ + html_body = "

This paragraph has no tags to remove.

" + expected_output = "

This paragraph has no tags to remove.

" + + result = clean_thread_html_body(html_body) + self.assertEqual(result, expected_output) + + def test_empty_html_body(self): + """ + Test that the clean_thread_html_body function returns an empty string if the input is an empty string + """ + html_body = "" + expected_output = "" + + result = clean_thread_html_body(html_body) + self.assertEqual(result, expected_output) + + def test_only_script_tag(self): + """ + Test that the clean_thread_html_body function removes the script tag and its content + """ + html_body = "" + expected_output = "alert('Hello');" + + result = clean_thread_html_body(html_body) + self.assertEqual(result.strip(), expected_output) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py index 28cfe3395cb6..8efd5cd49cbd 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py @@ -273,6 +273,17 @@ def setUp(self): }) self._register_subscriptions_endpoint() + self.comment = ThreadMock(thread_id=4, creator=self.user_2, title='test comment', body='comment body') + self.register_get_comment_response( + { + 'id': self.comment.id, + 'thread_id': self.thread.id, + 'parent_id': None, + 'user_id': self.comment.user_id, + 'body': self.comment.body, + } + ) + def test_basic(self): """ Left empty intentionally. This test case is inherited from DiscussionAPIViewTestMixin @@ -292,7 +303,13 @@ def test_send_notification_to_thread_creator(self): # Post the form or do what it takes to send the signal - send_response_notifications(self.thread.id, str(self.course.id), self.user_2.id, parent_id=None) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_2.id, + self.comment.id, + parent_id=None + ) self.assertEqual(handler.call_count, 2) args = handler.call_args_list[0][1]['notification_data'] self.assertEqual([int(user_id) for user_id in args.user_ids], [self.user_1.id]) @@ -300,6 +317,7 @@ def test_send_notification_to_thread_creator(self): expected_context = { 'replier_name': self.user_2.username, 'post_title': 'test thread', + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_2.id } @@ -325,7 +343,13 @@ def test_send_notification_to_parent_threads(self): 'user_id': self.thread_2.user_id }) - send_response_notifications(self.thread.id, str(self.course.id), self.user_3.id, parent_id=self.thread_2.id) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_3.id, + self.comment.id, + parent_id=self.thread_2.id + ) # check if 2 call are made to the handler i.e. one for the response creator and one for the thread creator self.assertEqual(handler.call_count, 2) @@ -337,6 +361,7 @@ def test_send_notification_to_parent_threads(self): expected_context = { 'replier_name': self.user_3.username, 'post_title': self.thread.title, + 'email_content': self.comment.body, 'author_name': 'dummy\'s', 'author_pronoun': 'dummy\'s', 'course_name': self.course.display_name, @@ -355,6 +380,7 @@ def test_send_notification_to_parent_threads(self): expected_context = { 'replier_name': self.user_3.username, 'post_title': self.thread.title, + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_3.id } @@ -372,7 +398,13 @@ def test_no_signal_on_creators_own_thread(self): """ handler = mock.Mock() USER_NOTIFICATION_REQUESTED.connect(handler) - send_response_notifications(self.thread.id, str(self.course.id), self.user_1.id, parent_id=None) + + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_1.id, + self.comment.id, parent_id=None + ) self.assertEqual(handler.call_count, 1) def test_comment_creators_own_response(self): @@ -389,7 +421,13 @@ def test_comment_creators_own_response(self): 'user_id': self.thread_3.user_id }) - send_response_notifications(self.thread.id, str(self.course.id), self.user_3.id, parent_id=self.thread_2.id) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_3.id, + parent_id=self.thread_2.id, + comment_id=self.comment.id + ) # check if 1 call is made to the handler i.e. for the thread creator self.assertEqual(handler.call_count, 2) @@ -404,6 +442,7 @@ def test_comment_creators_own_response(self): 'author_pronoun': 'your', 'course_name': self.course.display_name, 'sender_id': self.user_3.id, + 'email_content': self.comment.body } self.assertDictEqual(args_comment.context, expected_context) self.assertEqual( @@ -429,7 +468,13 @@ def test_send_notification_to_followers(self, parent_id, notification_type): USER_NOTIFICATION_REQUESTED.connect(handler) # Post the form or do what it takes to send the signal - notification_sender = DiscussionNotificationSender(self.thread, self.course, self.user_2, parent_id=parent_id) + notification_sender = DiscussionNotificationSender( + self.thread, + self.course, + self.user_2, + parent_id=parent_id, + comment_id=self.comment.id + ) notification_sender.send_response_on_followed_post_notification() self.assertEqual(handler.call_count, 1) args = handler.call_args[1]['notification_data'] @@ -439,6 +484,7 @@ def test_send_notification_to_followers(self, parent_id, notification_type): expected_context = { 'replier_name': self.user_2.username, 'post_title': 'test thread', + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_2.id, } @@ -516,6 +562,7 @@ def test_new_comment_notification(self): thread = ThreadMock(thread_id=1, creator=self.user_1, title='test thread') response = ThreadMock(thread_id=2, creator=self.user_2, title='test response') + comment = ThreadMock(thread_id=3, creator=self.user_2, title='test comment', body='comment body') self.register_get_thread_response({ 'id': thread.id, 'course_id': str(self.course.id), @@ -530,12 +577,20 @@ def test_new_comment_notification(self): 'thread_id': thread.id, 'user_id': response.user_id }) + self.register_get_comment_response({ + 'id': comment.id, + 'parent_id': response.id, + 'user_id': comment.user_id, + 'body': comment.body + }) self.register_get_subscriptions(1, {}) - send_response_notifications(thread.id, str(self.course.id), self.user_2.id, parent_id=response.id) + send_response_notifications(thread.id, str(self.course.id), self.user_2.id, parent_id=response.id, + comment_id=comment.id) handler.assert_called_once() context = handler.call_args[1]['notification_data'].context self.assertEqual(context['author_name'], 'dummy\'s') self.assertEqual(context['author_pronoun'], 'their') + self.assertEqual(context['email_content'], comment.body) @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index 989fd63855d5..27e34705f5df 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -675,12 +675,13 @@ class ThreadMock(object): A mock thread object """ - def __init__(self, thread_id, creator, title, parent_id=None): + def __init__(self, thread_id, creator, title, parent_id=None, body=''): self.id = thread_id self.user_id = str(creator.id) self.username = creator.username self.title = title self.parent_id = parent_id + self.body = body def url_with_id(self, params): return f"http://example.com/{params['id']}" diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index 35288cdbd9be..2aa7d36456c4 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -176,8 +176,9 @@ def create_comment_created_notification(*args, **kwargs): comment = kwargs['post'] thread_id = comment.attributes['thread_id'] parent_id = comment.attributes['parent_id'] + comment_id = comment.attributes['id'] course_key_str = comment.attributes['course_id'] - send_response_notifications.apply_async(args=[thread_id, course_key_str, user.id, parent_id]) + send_response_notifications.apply_async(args=[thread_id, course_key_str, user.id, comment_id, parent_id]) @receiver(signals.comment_endorsed) From 353dc34d9c0f23f3e3e5ff0d024b5b3de0ccf44d Mon Sep 17 00:00:00 2001 From: Mudassir Hafeez Date: Mon, 26 Aug 2024 15:53:27 +0500 Subject: [PATCH 07/12] chore: provide logo url from backend for batch enrollment email (#35138) * chore: provide logo url from backend for batch enrollment email --- lms/djangoapps/instructor/enrollment.py | 2 ++ .../instructor/tests/test_enrollment.py | 16 ++++++++++++++++ .../ace_common/edx_ace/common/base_body.html | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 300543def6c2..896d0deadcd9 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -34,6 +34,7 @@ get_event_transaction_id, set_event_transaction_type ) +from lms.djangoapps.branding.api import get_logo_url_for_email from lms.djangoapps.courseware.models import StudentModule from lms.djangoapps.grades.api import constants as grades_constants from lms.djangoapps.grades.api import disconnect_submissions_signal_receiver @@ -489,6 +490,7 @@ def get_email_params(course, auto_enroll, secure=True, course_key=None, display_ 'contact_mailing_address': contact_mailing_address, 'platform_name': platform_name, 'site_configuration_values': configuration_helpers.get_current_site_configuration_values(), + 'logo_url': get_logo_url_for_email(), } return email_params diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 59ccfac6caa1..741f57ef6d2b 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -23,6 +23,7 @@ from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed, anonymous_id_for_user from common.djangoapps.student.roles import CourseCcxCoachRole from common.djangoapps.student.tests.factories import AdminFactory, UserFactory +from lms.djangoapps.branding.api import get_logo_url_for_email from lms.djangoapps.ccx.tests.factories import CcxFactory from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.courseware.models import StudentModule @@ -940,6 +941,7 @@ def setUpClass(cls): ) cls.course_about_url = cls.course_url + 'about' cls.registration_url = f'https://{site}/register' + cls.logo_url = get_logo_url_for_email() def test_normal_params(self): # For a normal site, what do we expect to get for the URLs? @@ -950,6 +952,7 @@ def test_normal_params(self): assert result['course_about_url'] == self.course_about_url assert result['registration_url'] == self.registration_url assert result['course_url'] == self.course_url + assert result['logo_url'] == self.logo_url def test_marketing_params(self): # For a site with a marketing front end, what do we expect to get for the URLs? @@ -962,6 +965,19 @@ def test_marketing_params(self): assert result['course_about_url'] is None assert result['registration_url'] == self.registration_url assert result['course_url'] == self.course_url + assert result['logo_url'] == self.logo_url + + @patch('lms.djangoapps.instructor.enrollment.get_logo_url_for_email', return_value='https://www.logo.png') + def test_logo_url_params(self, mock_get_logo_url_for_email): + # Verify that the logo_url is correctly set in the email params + result = get_email_params(self.course, False) + + assert result['auto_enroll'] is False + assert result['course_about_url'] == self.course_about_url + assert result['registration_url'] == self.registration_url + assert result['course_url'] == self.course_url + mock_get_logo_url_for_email.assert_called_once() + assert result['logo_url'] == 'https://www.logo.png' @ddt.ddt diff --git a/themes/red-theme/lms/templates/ace_common/edx_ace/common/base_body.html b/themes/red-theme/lms/templates/ace_common/edx_ace/common/base_body.html index 8d51b16498d7..9319217aa4cf 100644 --- a/themes/red-theme/lms/templates/ace_common/edx_ace/common/base_body.html +++ b/themes/red-theme/lms/templates/ace_common/edx_ace/common/base_body.html @@ -63,7 +63,7 @@ {% filter force_escape %} {% blocktrans %}Go to {{ platform_name }} Home Page{% endblocktrans %} {% endfilter %} From 0f177e466639bc8e0dc4acece00a7e82e7ae6001 Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Mon, 26 Aug 2024 10:20:03 -0400 Subject: [PATCH 08/12] feat: linting only (#35370) Editing this file is going to cause a lot of automatic linting, so lint-only commit for nicer code review. --- .../djangoapps/user_api/accounts/views.py | 723 +++++++++--------- 1 file changed, 353 insertions(+), 370 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 720b3ba96af7..103c5bf24f6c 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -21,7 +21,6 @@ from edx_ace import ace from edx_ace.recipient import Recipient from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication -from openedx.core.lib.api.authentication import BearerAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from enterprise.models import EnterpriseCourseEnrollment, EnterpriseCustomerUser, PendingEnterpriseCustomerUser from integrated_channels.degreed.models import DegreedLearnerDataTransmissionAudit @@ -50,9 +49,10 @@ get_retired_email_by_email, get_retired_username_by_username, is_email_retired, - is_username_retired + is_username_retired, ) from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_pending_name_change +from lms.djangoapps.certificates.api import clear_pii_from_certificate_records_for_user from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.api_admin.models import ApiAccessRequest from openedx.core.djangoapps.course_groups.models import UnregisteredLearnerCohortAssignments @@ -64,9 +64,8 @@ from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_names, set_has_profile_image from openedx.core.djangoapps.user_api.accounts.utils import handle_retirement_cancellation from openedx.core.djangoapps.user_authn.exceptions import AuthFailedError -from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser +from openedx.core.lib.api.authentication import BearerAuthentication, BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.parsers import MergePatchParser -from lms.djangoapps.certificates.api import clear_pii_from_certificate_records_for_user from ..errors import AccountUpdateError, AccountValidationError, UserNotAuthorized, UserNotFound from ..message_types import DeletionNotificationMessage @@ -75,7 +74,7 @@ RetirementStateError, UserOrgTag, UserRetirementPartnerReportingStatus, - UserRetirementStatus + UserRetirementStatus, ) from .api import get_account_settings, update_account_settings from .permissions import ( @@ -83,13 +82,13 @@ CanDeactivateUser, CanGetAccountInfo, CanReplaceUsername, - CanRetireUser + CanRetireUser, ) from .serializers import ( PendingNameChangeSerializer, UserRetirementPartnerReportSerializer, UserRetirementStatusSerializer, - UserSearchEmailSerializer + UserSearchEmailSerializer, ) from .signals import USER_RETIRE_LMS_CRITICAL, USER_RETIRE_LMS_MISC, USER_RETIRE_MAILINGS from .utils import create_retirement_request_and_deactivate_account, username_suffix_generator @@ -97,16 +96,16 @@ log = logging.getLogger(__name__) USER_PROFILE_PII = { - 'name': '', - 'meta': '', - 'location': '', - 'year_of_birth': None, - 'gender': None, - 'mailing_address': None, - 'city': None, - 'country': None, - 'bio': None, - 'phone_number': None, + "name": "", + "meta": "", + "location": "", + "year_of_birth": None, + "gender": None, + "mailing_address": None, + "city": None, + "country": None, + "bio": None, + "phone_number": None, } @@ -118,12 +117,9 @@ def request_requires_username(function): @wraps(function) def wrapper(self, request): # pylint: disable=missing-docstring - username = request.data.get('username', None) + username = request.data.get("username", None) if not username: - return Response( - status=status.HTTP_404_NOT_FOUND, - data={'message': 'The user was not specified.'} - ) + return Response(status=status.HTTP_404_NOT_FOUND, data={"message": "The user was not specified."}) return function(self, request) return wrapper @@ -131,177 +127,183 @@ def wrapper(self, request): # pylint: disable=missing-docstring class AccountViewSet(ViewSet): """ - **Use Cases** - - Get or update a user's account information. Updates are supported - only through merge patch. - - **Example Requests** - - GET /api/user/v1/me[?view=shared] - GET /api/user/v1/accounts?usernames={username1,username2}[?view=shared] - GET /api/user/v1/accounts?email={user_email} - GET /api/user/v1/accounts/{username}/[?view=shared] - - PATCH /api/user/v1/accounts/{username}/{"key":"value"} "application/merge-patch+json" - - POST /api/user/v1/accounts/search_emails "application/json" - - **Notes for PATCH requests to /accounts endpoints** - * Requested updates to social_links are automatically merged with - previously set links. That is, any newly introduced platforms are - add to the previous list. Updated links to pre-existing platforms - replace their values in the previous list. Pre-existing platforms - can be removed by setting the value of the social_link to an - empty string (""). - - **Response Values for GET requests to the /me endpoint** - If the user is not logged in, an HTTP 401 "Not Authorized" response - is returned. - - Otherwise, an HTTP 200 "OK" response is returned. The response - contains the following value: - - * username: The username associated with the account. - - **Response Values for GET requests to /accounts endpoints** - - If no user exists with the specified username, or email, an HTTP 404 "Not - Found" response is returned. - - If the user makes the request for her own account, or makes a - request for another account and has "is_staff" access, an HTTP 200 - "OK" response is returned. The response contains the following - values. - - * id: numerical lms user id in db - * activation_key: auto-genrated activation key when signed up via email - * bio: null or textual representation of user biographical - information ("about me"). - * country: An ISO 3166 country code or null. - * date_joined: The date the account was created, in the string - format provided by datetime. For example, "2014-08-26T17:52:11Z". - * last_login: The latest date the user logged in, in the string datetime format. - * email: Email address for the user. New email addresses must be confirmed - via a confirmation email, so GET does not reflect the change until - the address has been confirmed. - * secondary_email: A secondary email address for the user. Unlike - the email field, GET will reflect the latest update to this field - even if changes have yet to be confirmed. - * verified_name: Approved verified name of the learner present in name affirmation plugin - * gender: One of the following values: - - * null - * "f" - * "m" - * "o" - - * goals: The textual representation of the user's goals, or null. - * is_active: Boolean representation of whether a user is active. - * language: The user's preferred language, or null. - * language_proficiencies: Array of language preferences. Each - preference is a JSON object with the following keys: - - * "code": string ISO 639-1 language code e.g. "en". - - * level_of_education: One of the following values: - - * "p": PhD or Doctorate - * "m": Master's or professional degree - * "b": Bachelor's degree - * "a": Associate's degree - * "hs": Secondary/high school - * "jhs": Junior secondary/junior high/middle school - * "el": Elementary/primary school - * "none": None - * "o": Other - * null: The user did not enter a value - - * mailing_address: The textual representation of the user's mailing - address, or null. - * name: The full name of the user. - * profile_image: A JSON representation of a user's profile image - information. This representation has the following keys. - - * "has_image": Boolean indicating whether the user has a profile - image. - * "image_url_*": Absolute URL to various sizes of a user's - profile image, where '*' matches a representation of the - corresponding image size, such as 'small', 'medium', 'large', - and 'full'. These are configurable via PROFILE_IMAGE_SIZES_MAP. - - * requires_parental_consent: True if the user is a minor - requiring parental consent. - * social_links: Array of social links, sorted alphabetically by - "platform". Each preference is a JSON object with the following keys: - - * "platform": A particular social platform, ex: 'facebook' - * "social_link": The link to the user's profile on the particular platform - - * username: The username associated with the account. - * year_of_birth: The year the user was born, as an integer, or null. - - * account_privacy: The user's setting for sharing her personal - profile. Possible values are "all_users", "private", or "custom". - If "custom", the user has selectively chosen a subset of shareable - fields to make visible to others via the User Preferences API. - - * phone_number: The phone number for the user. String of numbers with - an optional `+` sign at the start. - - * pending_name_change: If the user has an active name change request, returns the - requested name. - - For all text fields, plain text instead of HTML is supported. The - data is stored exactly as specified. Clients must HTML escape - rendered values to avoid script injections. - - If a user who does not have "is_staff" access requests account - information for a different user, only a subset of these fields is - returned. The returned fields depend on the - ACCOUNT_VISIBILITY_CONFIGURATION configuration setting and the - visibility preference of the user for whom data is requested. - - Note that a user can view which account fields they have shared - with other users by requesting their own username and providing - the "view=shared" URL parameter. - - **Response Values for PATCH** - - Users can only modify their own account information. If the - requesting user does not have the specified username and has staff - access, the request returns an HTTP 403 "Forbidden" response. If - the requesting user does not have staff access, the request - returns an HTTP 404 "Not Found" response to avoid revealing the - existence of the account. - - If no user exists with the specified username, an HTTP 404 "Not - Found" response is returned. - - If "application/merge-patch+json" is not the specified content - type, a 415 "Unsupported Media Type" response is returned. - - If validation errors prevent the update, this method returns a 400 - "Bad Request" response that includes a "field_errors" field that - lists all error messages. - - If a failure at the time of the update prevents the update, a 400 - "Bad Request" error is returned. The JSON collection contains - specific errors. - - If the update is successful, updated user account data is returned. + **Use Cases** + + Get or update a user's account information. Updates are supported + only through merge patch. + + **Example Requests** + + GET /api/user/v1/me[?view=shared] + GET /api/user/v1/accounts?usernames={username1,username2}[?view=shared] + GET /api/user/v1/accounts?email={user_email} + GET /api/user/v1/accounts/{username}/[?view=shared] + + PATCH /api/user/v1/accounts/{username}/{"key":"value"} "application/merge-patch+json" + + POST /api/user/v1/accounts/search_emails "application/json" + + **Notes for PATCH requests to /accounts endpoints** + * Requested updates to social_links are automatically merged with + previously set links. That is, any newly introduced platforms are + add to the previous list. Updated links to pre-existing platforms + replace their values in the previous list. Pre-existing platforms + can be removed by setting the value of the social_link to an + empty string (""). + + **Response Values for GET requests to the /me endpoint** + If the user is not logged in, an HTTP 401 "Not Authorized" response + is returned. + + Otherwise, an HTTP 200 "OK" response is returned. The response + contains the following value: + + * username: The username associated with the account. + + **Response Values for GET requests to /accounts endpoints** + + If no user exists with the specified username, or email, an HTTP 404 "Not + Found" response is returned. + + If the user makes the request for her own account, or makes a + request for another account and has "is_staff" access, an HTTP 200 + "OK" response is returned. The response contains the following + values. + + * id: numerical lms user id in db + * activation_key: auto-genrated activation key when signed up via email + * bio: null or textual representation of user biographical + information ("about me"). + * country: An ISO 3166 country code or null. + * date_joined: The date the account was created, in the string + format provided by datetime. For example, "2014-08-26T17:52:11Z". + * last_login: The latest date the user logged in, in the string datetime format. + * email: Email address for the user. New email addresses must be confirmed + via a confirmation email, so GET does not reflect the change until + the address has been confirmed. + * secondary_email: A secondary email address for the user. Unlike + the email field, GET will reflect the latest update to this field + even if changes have yet to be confirmed. + * verified_name: Approved verified name of the learner present in name affirmation plugin + * gender: One of the following values: + + * null + * "f" + * "m" + * "o" + + * goals: The textual representation of the user's goals, or null. + * is_active: Boolean representation of whether a user is active. + * language: The user's preferred language, or null. + * language_proficiencies: Array of language preferences. Each + preference is a JSON object with the following keys: + + * "code": string ISO 639-1 language code e.g. "en". + + * level_of_education: One of the following values: + + * "p": PhD or Doctorate + * "m": Master's or professional degree + * "b": Bachelor's degree + * "a": Associate's degree + * "hs": Secondary/high school + * "jhs": Junior secondary/junior high/middle school + * "el": Elementary/primary school + * "none": None + * "o": Other + * null: The user did not enter a value + + * mailing_address: The textual representation of the user's mailing + address, or null. + * name: The full name of the user. + * profile_image: A JSON representation of a user's profile image + information. This representation has the following keys. + + * "has_image": Boolean indicating whether the user has a profile + image. + * "image_url_*": Absolute URL to various sizes of a user's + profile image, where '*' matches a representation of the + corresponding image size, such as 'small', 'medium', 'large', + and 'full'. These are configurable via PROFILE_IMAGE_SIZES_MAP. + + * requires_parental_consent: True if the user is a minor + requiring parental consent. + * social_links: Array of social links, sorted alphabetically by + "platform". Each preference is a JSON object with the following keys: + + * "platform": A particular social platform, ex: 'facebook' + * "social_link": The link to the user's profile on the particular platform + + * username: The username associated with the account. + * year_of_birth: The year the user was born, as an integer, or null. + + * account_privacy: The user's setting for sharing her personal + profile. Possible values are "all_users", "private", or "custom". + If "custom", the user has selectively chosen a subset of shareable + fields to make visible to others via the User Preferences API. + + * phone_number: The phone number for the user. String of numbers with + an optional `+` sign at the start. + + * pending_name_change: If the user has an active name change request, returns the + requested name. + + For all text fields, plain text instead of HTML is supported. The + data is stored exactly as specified. Clients must HTML escape + rendered values to avoid script injections. + + If a user who does not have "is_staff" access requests account + information for a different user, only a subset of these fields is + returned. The returned fields depend on the + ACCOUNT_VISIBILITY_CONFIGURATION configuration setting and the + visibility preference of the user for whom data is requested. + + Note that a user can view which account fields they have shared + with other users by requesting their own username and providing + the "view=shared" URL parameter. + + **Response Values for PATCH** + + Users can only modify their own account information. If the + requesting user does not have the specified username and has staff + access, the request returns an HTTP 403 "Forbidden" response. If + the requesting user does not have staff access, the request + returns an HTTP 404 "Not Found" response to avoid revealing the + existence of the account. + + If no user exists with the specified username, an HTTP 404 "Not + Found" response is returned. + + If "application/merge-patch+json" is not the specified content + type, a 415 "Unsupported Media Type" response is returned. + + If validation errors prevent the update, this method returns a 400 + "Bad Request" response that includes a "field_errors" field that + lists all error messages. + + If a failure at the time of the update prevents the update, a 400 + "Bad Request" error is returned. The JSON collection contains + specific errors. + + If the update is successful, updated user account data is returned. """ + authentication_classes = ( - JwtAuthentication, BearerAuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, ) permission_classes = (permissions.IsAuthenticated, CanGetAccountInfo) - parser_classes = (JSONParser, MergePatchParser,) + parser_classes = ( + JSONParser, + MergePatchParser, + ) def get(self, request): """ GET /api/user/v1/me """ - return Response({'username': request.user.username}) + return Response({"username": request.user.username}) def list(self, request): """ @@ -309,13 +311,13 @@ def list(self, request): GET /api/user/v1/accounts?email={user_email} (Staff Only) GET /api/user/v1/accounts?lms_user_id={lms_user_id} (Staff Only) """ - usernames = request.GET.get('username') - user_email = request.GET.get('email') - lms_user_id = request.GET.get('lms_user_id') + usernames = request.GET.get("username") + user_email = request.GET.get("email") + lms_user_id = request.GET.get("lms_user_id") search_usernames = [] if usernames: - search_usernames = usernames.strip(',').split(',') + search_usernames = usernames.strip(",").split(",") elif user_email: if is_email_retired(user_email): can_cancel_retirement = True @@ -325,22 +327,20 @@ def list(self, request): retirement_status = UserRetirementStatus.objects.get( created__gt=earliest_datetime, created__lt=datetime.datetime.now(pytz.UTC), - original_email=user_email + original_email=user_email, ) retirement_id = retirement_status.id except UserRetirementStatus.DoesNotExist: can_cancel_retirement = False context = { - 'error_msg': accounts.RETIRED_EMAIL_MSG, - 'can_cancel_retirement': can_cancel_retirement, - 'retirement_id': retirement_id + "error_msg": accounts.RETIRED_EMAIL_MSG, + "can_cancel_retirement": can_cancel_retirement, + "retirement_id": retirement_id, } - return Response( - context, status=status.HTTP_404_NOT_FOUND - ) - user_email = user_email.strip('') + return Response(context, status=status.HTTP_404_NOT_FOUND) + user_email = user_email.strip("") try: user = User.objects.get(email=user_email) except (UserNotFound, User.DoesNotExist): @@ -355,9 +355,7 @@ def list(self, request): return Response(status=status.HTTP_400_BAD_REQUEST) search_usernames = [user.username] try: - account_settings = get_account_settings( - request, search_usernames, view=request.query_params.get('view') - ) + account_settings = get_account_settings(request, search_usernames, view=request.query_params.get("view")) except UserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) @@ -386,23 +384,15 @@ def search_emails(self, request): """ if not request.user.is_staff: return Response( - { - 'developer_message': 'not_found', - 'user_message': 'Not Found' - }, - status=status.HTTP_404_NOT_FOUND + {"developer_message": "not_found", "user_message": "Not Found"}, status=status.HTTP_404_NOT_FOUND ) try: - user_emails = request.data['emails'] + user_emails = request.data["emails"] except KeyError as error: - error_message = f'{error} field is required' + error_message = f"{error} field is required" return Response( - { - 'developer_message': error_message, - 'user_message': error_message - }, - status=status.HTTP_400_BAD_REQUEST + {"developer_message": error_message, "user_message": error_message}, status=status.HTTP_400_BAD_REQUEST ) users = User.objects.filter(email__in=user_emails) data = UserSearchEmailSerializer(users, many=True).data @@ -413,8 +403,7 @@ def retrieve(self, request, username): GET /api/user/v1/accounts/{username}/ """ try: - account_settings = get_account_settings( - request, [username], view=request.query_params.get('view')) + account_settings = get_account_settings(request, [username], view=request.query_params.get("view")) except UserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) @@ -443,11 +432,8 @@ def partial_update(self, request, username): return Response({"field_errors": err.field_errors}, status=status.HTTP_400_BAD_REQUEST) except AccountUpdateError as err: return Response( - { - "developer_message": err.developer_message, - "user_message": err.user_message - }, - status=status.HTTP_400_BAD_REQUEST + {"developer_message": err.developer_message, "user_message": err.user_message}, + status=status.HTTP_400_BAD_REQUEST, ) return Response(account_settings) @@ -457,6 +443,7 @@ class NameChangeView(ViewSet): """ Viewset to manage profile name change requests. """ + permission_classes = (permissions.IsAuthenticated,) def create(self, request): @@ -472,10 +459,10 @@ def create(self, request): } """ user = request.user - new_name = request.data.get('name', None) - rationale = f'Name change requested through account API by {user.username}' + new_name = request.data.get("name", None) + rationale = f"Name change requested through account API by {user.username}" - serializer = PendingNameChangeSerializer(data={'new_name': new_name}) + serializer = PendingNameChangeSerializer(data={"new_name": new_name}) if serializer.is_valid(): pending_name_change = do_name_change_request(user, new_name, rationale)[0] @@ -483,8 +470,8 @@ def create(self, request): return Response(status=status.HTTP_201_CREATED) else: return Response( - {'new_name': 'The profile name given was identical to the current name.'}, - status=status.HTTP_400_BAD_REQUEST + {"new_name": "The profile name given was identical to the current name."}, + status=status.HTTP_400_BAD_REQUEST, ) return Response(status=status.HTTP_400_BAD_REQUEST, data=serializer.errors) @@ -514,6 +501,7 @@ class AccountDeactivationView(APIView): Account deactivation viewset. Currently only supports POST requests. Only admins can deactivate accounts. """ + permission_classes = (permissions.IsAuthenticated, CanDeactivateUser) def post(self, request, username): @@ -559,6 +547,7 @@ class DeactivateLogoutView(APIView): - Log the user out - Create a row in the retirement table for that user """ + # BearerAuthentication is added here to support account deletion # from the mobile app until it moves to JWT Auth. # See mobile roadmap issue https://github.com/openedx/edx-platform/issues/33307. @@ -575,7 +564,7 @@ def post(self, request): # Ensure the account deletion is not disable enable_account_deletion = configuration_helpers.get_value( - 'ENABLE_ACCOUNT_DELETION', settings.FEATURES.get('ENABLE_ACCOUNT_DELETION', False) + "ENABLE_ACCOUNT_DELETION", settings.FEATURES.get("ENABLE_ACCOUNT_DELETION", False) ) if not enable_account_deletion: @@ -595,11 +584,9 @@ def post(self, request): # Send notification email to user site = Site.objects.get_current() notification_context = get_base_template_context(site) - notification_context.update({'full_name': request.user.profile.name}) + notification_context.update({"full_name": request.user.profile.name}) language_code = request.user.preferences.model.get_value( - request.user, - LANGUAGE_KEY, - default=settings.LANGUAGE_CODE + request.user, LANGUAGE_KEY, default=settings.LANGUAGE_CODE ) notification = DeletionNotificationMessage().personalize( recipient=Recipient(lms_user_id=0, email_address=user_email), @@ -608,22 +595,20 @@ def post(self, request): ) ace.send(notification) except Exception as exc: - log.exception('Error sending out deletion notification email') + log.exception("Error sending out deletion notification email") raise exc # Log the user out. logout(request) return Response(status=status.HTTP_204_NO_CONTENT) except KeyError: - log.exception(f'Username not specified {request.user}') - return Response('Username not specified.', status=status.HTTP_404_NOT_FOUND) + log.exception(f"Username not specified {request.user}") + return Response("Username not specified.", status=status.HTTP_404_NOT_FOUND) except user_model.DoesNotExist: log.exception(f'The user "{request.user.username}" does not exist.') - return Response( - f'The user "{request.user.username}" does not exist.', status=status.HTTP_404_NOT_FOUND - ) + return Response(f'The user "{request.user.username}" does not exist.', status=status.HTTP_404_NOT_FOUND) except Exception as exc: # pylint: disable=broad-except - log.exception(f'500 error deactivating account {exc}') + log.exception(f"500 error deactivating account {exc}") return Response(str(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) def _verify_user_password(self, request): @@ -636,7 +621,7 @@ def _verify_user_password(self, request): """ try: self._check_excessive_login_attempts(request.user) - user = authenticate(username=request.user.username, password=request.POST['password'], request=request) + user = authenticate(username=request.user.username, password=request.POST["password"], request=request) if user: if LoginFailures.is_feature_enabled(): LoginFailures.clear_lockout_counter(user) @@ -644,9 +629,7 @@ def _verify_user_password(self, request): else: self._handle_failed_authentication(request.user) except AuthFailedError as err: - log.exception( - f"The user password to deactivate was incorrect. {request.user.username}" - ) + log.exception(f"The user password to deactivate was incorrect. {request.user.username}") return Response(str(err), status=status.HTTP_403_FORBIDDEN) except Exception as err: # pylint: disable=broad-except return Response(f"Could not verify user password: {err}", status=status.HTTP_400_BAD_REQUEST) @@ -657,8 +640,9 @@ def _check_excessive_login_attempts(self, user): """ if user and LoginFailures.is_feature_enabled(): if LoginFailures.is_user_locked_out(user): - raise AuthFailedError(_('This account has been temporarily locked due ' - 'to excessive login failures. Try again later.')) + raise AuthFailedError( + _("This account has been temporarily locked due to excessive login failures. Try again later.") + ) def _handle_failed_authentication(self, user): """ @@ -667,7 +651,7 @@ def _handle_failed_authentication(self, user): if user and LoginFailures.is_feature_enabled(): LoginFailures.increment_lockout_counter(user) - raise AuthFailedError(_('Email or password is incorrect.')) + raise AuthFailedError(_("Email or password is incorrect.")) def _set_unusable_password(user): @@ -684,15 +668,19 @@ class AccountRetirementPartnerReportView(ViewSet): Provides API endpoints for managing partner reporting of retired users. """ - DELETION_COMPLETED_KEY = 'deletion_completed' - ORGS_CONFIG_KEY = 'orgs_config' - ORGS_CONFIG_ORG_KEY = 'org' - ORGS_CONFIG_FIELD_HEADINGS_KEY = 'field_headings' - ORIGINAL_EMAIL_KEY = 'original_email' - ORIGINAL_NAME_KEY = 'original_name' - STUDENT_ID_KEY = 'student_id' - - permission_classes = (permissions.IsAuthenticated, CanRetireUser,) + + DELETION_COMPLETED_KEY = "deletion_completed" + ORGS_CONFIG_KEY = "orgs_config" + ORGS_CONFIG_ORG_KEY = "org" + ORGS_CONFIG_FIELD_HEADINGS_KEY = "field_headings" + ORIGINAL_EMAIL_KEY = "original_email" + ORIGINAL_NAME_KEY = "original_name" + STUDENT_ID_KEY = "student_id" + + permission_classes = ( + permissions.IsAuthenticated, + CanRetireUser, + ) parser_classes = (JSONParser,) serializer_class = UserRetirementStatusSerializer @@ -706,7 +694,7 @@ def _get_orgs_for_user(user): org = enrollment.course_id.org # Org can conceivably be blank or this bogus default value - if org and org != 'outdated_entry': + if org and org != "outdated_entry": orgs.add(org) return orgs @@ -718,9 +706,9 @@ def retirement_partner_report(self, request): # pylint: disable=unused-argument that are not already being processed and updates their status to indicate they are currently being processed. """ - retirement_statuses = UserRetirementPartnerReportingStatus.objects.filter( - is_being_processed=False - ).order_by('id') + retirement_statuses = UserRetirementPartnerReportingStatus.objects.filter(is_being_processed=False).order_by( + "id" + ) retirements = [] for retirement_status in retirement_statuses: @@ -737,12 +725,12 @@ def _get_retirement_for_partner_report(self, retirement_status): Get the retirement for this retirement_status. The retirement info will be included in the partner report. """ retirement = { - 'user_id': retirement_status.user.pk, - 'original_username': retirement_status.original_username, + "user_id": retirement_status.user.pk, + "original_username": retirement_status.original_username, AccountRetirementPartnerReportView.ORIGINAL_EMAIL_KEY: retirement_status.original_email, AccountRetirementPartnerReportView.ORIGINAL_NAME_KEY: retirement_status.original_name, - 'orgs': self._get_orgs_for_user(retirement_status.user), - 'created': retirement_status.created, + "orgs": self._get_orgs_for_user(retirement_status.user), + "created": retirement_status.created, } return retirement @@ -761,7 +749,7 @@ def retirement_partner_status_create(self, request): Creates a UserRetirementPartnerReportingStatus object for the given user as part of the retirement pipeline. """ - username = request.data['username'] + username = request.data["username"] try: retirement = UserRetirementStatus.get_retirement_for_retirement_action(username) @@ -771,10 +759,10 @@ def retirement_partner_status_create(self, request): UserRetirementPartnerReportingStatus.objects.get_or_create( user=retirement.user, defaults={ - 'original_username': retirement.original_username, - 'original_email': retirement.original_email, - 'original_name': retirement.original_name - } + "original_username": retirement.original_username, + "original_email": retirement.original_email, + "original_name": retirement.original_name, + }, ) return Response(status=status.HTTP_204_NO_CONTENT) @@ -790,14 +778,13 @@ def retirement_partner_cleanup(self, request): Deletes UserRetirementPartnerReportingStatus objects for a list of users that have been reported on. """ - usernames = [u['original_username'] for u in request.data] + usernames = [u["original_username"] for u in request.data] if not usernames: - return Response('No original_usernames given.', status=status.HTTP_400_BAD_REQUEST) + return Response("No original_usernames given.", status=status.HTTP_400_BAD_REQUEST) retirement_statuses = UserRetirementPartnerReportingStatus.objects.filter( - is_being_processed=True, - original_username__in=usernames + is_being_processed=True, original_username__in=usernames ) # Need to de-dupe usernames that differ only by case to find the exact right match @@ -809,15 +796,15 @@ def retirement_partner_cleanup(self, request): # to disambiguate them in Python, which will respect case in the comparison. if len(usernames) != len(retirement_statuses_clean): return Response( - '{} original_usernames given, {} found!\n' - 'Given usernames:\n{}\n' - 'Found UserRetirementReportingStatuses:\n{}'.format( + "{} original_usernames given, {} found!\n" + "Given usernames:\n{}\n" + "Found UserRetirementReportingStatuses:\n{}".format( len(usernames), len(retirement_statuses_clean), usernames, - ', '.join([rs.original_username for rs in retirement_statuses_clean]) + ", ".join([rs.original_username for rs in retirement_statuses_clean]), ), - status=status.HTTP_400_BAD_REQUEST + status=status.HTTP_400_BAD_REQUEST, ) retirement_statuses.delete() @@ -829,7 +816,11 @@ class CancelAccountRetirementStatusView(ViewSet): """ Provides API endpoints for canceling retirement process for a user's account. """ - permission_classes = (permissions.IsAuthenticated, CanCancelUserRetirement,) + + permission_classes = ( + permissions.IsAuthenticated, + CanCancelUserRetirement, + ) def cancel_retirement(self, request): """ @@ -839,26 +830,23 @@ def cancel_retirement(self, request): This also handles the top level error handling, and permissions. """ try: - retirement_id = request.data['retirement_id'] + retirement_id = request.data["retirement_id"] except KeyError: - return Response( - status=status.HTTP_400_BAD_REQUEST, - data={'message': 'retirement_id must be specified.'} - ) + return Response(status=status.HTTP_400_BAD_REQUEST, data={"message": "retirement_id must be specified."}) try: retirement = UserRetirementStatus.objects.get(id=retirement_id) except UserRetirementStatus.DoesNotExist: - return Response(data={"message": 'Retirement does not exist!'}, status=status.HTTP_400_BAD_REQUEST) + return Response(data={"message": "Retirement does not exist!"}, status=status.HTTP_400_BAD_REQUEST) - if retirement.current_state.state_name != 'PENDING': + if retirement.current_state.state_name != "PENDING": return Response( status=status.HTTP_400_BAD_REQUEST, data={ "message": f"Retirement requests can only be cancelled for users in the PENDING state. Current " - f"request state for '{retirement.original_username}': " - f"{retirement.current_state.state_name}" - } + f"request state for '{retirement.original_username}': " + f"{retirement.current_state.state_name}" + }, ) handle_retirement_cancellation(retirement) @@ -870,7 +858,11 @@ class AccountRetirementStatusView(ViewSet): """ Provides API endpoints for managing the user retirement process. """ - permission_classes = (permissions.IsAuthenticated, CanRetireUser,) + + permission_classes = ( + permissions.IsAuthenticated, + CanRetireUser, + ) parser_classes = (JSONParser,) serializer_class = UserRetirementStatusSerializer @@ -883,37 +875,35 @@ def retirement_queue(self, request): created in the retirement queue at least `cool_off_days` ago. """ try: - cool_off_days = int(request.GET['cool_off_days']) + cool_off_days = int(request.GET["cool_off_days"]) if cool_off_days < 0: - raise RetirementStateError('Invalid argument for cool_off_days, must be greater than 0.') + raise RetirementStateError("Invalid argument for cool_off_days, must be greater than 0.") - states = request.GET.getlist('states') + states = request.GET.getlist("states") if not states: raise RetirementStateError('Param "states" required with at least one state.') state_objs = RetirementState.objects.filter(state_name__in=states) if state_objs.count() != len(states): found = [s.state_name for s in state_objs] - raise RetirementStateError(f'Unknown state. Requested: {states} Found: {found}') + raise RetirementStateError(f"Unknown state. Requested: {states} Found: {found}") - limit = request.GET.get('limit') + limit = request.GET.get("limit") if limit: try: limit_count = int(limit) except ValueError: return Response( - f'Limit could not be parsed: {limit}, please ensure this is an integer', - status=status.HTTP_400_BAD_REQUEST + f"Limit could not be parsed: {limit}, please ensure this is an integer", + status=status.HTTP_400_BAD_REQUEST, ) earliest_datetime = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=cool_off_days) - retirements = UserRetirementStatus.objects.select_related( - 'user', 'current_state', 'last_state' - ).filter( - current_state__in=state_objs, created__lt=earliest_datetime - ).order_by( - 'id' + retirements = ( + UserRetirementStatus.objects.select_related("user", "current_state", "last_state") + .filter(current_state__in=state_objs, created__lt=earliest_datetime) + .order_by("id") ) if limit: retirements = retirements[:limit_count] @@ -921,10 +911,9 @@ def retirement_queue(self, request): return Response(serializer.data) # This should only occur on the int() conversion of cool_off_days at this point except ValueError: - return Response('Invalid cool_off_days, should be integer.', status=status.HTTP_400_BAD_REQUEST) + return Response("Invalid cool_off_days, should be integer.", status=status.HTTP_400_BAD_REQUEST) except KeyError as exc: - return Response(f'Missing required parameter: {str(exc)}', - status=status.HTTP_400_BAD_REQUEST) + return Response(f"Missing required parameter: {str(exc)}", status=status.HTTP_400_BAD_REQUEST) except RetirementStateError as exc: return Response(str(exc), status=status.HTTP_400_BAD_REQUEST) @@ -939,36 +928,33 @@ def retirements_by_status_and_date(self, request): so to get one day you would set both dates to that day. """ try: - start_date = datetime.datetime.strptime(request.GET['start_date'], '%Y-%m-%d').replace(tzinfo=pytz.UTC) - end_date = datetime.datetime.strptime(request.GET['end_date'], '%Y-%m-%d').replace(tzinfo=pytz.UTC) + start_date = datetime.datetime.strptime(request.GET["start_date"], "%Y-%m-%d").replace(tzinfo=pytz.UTC) + end_date = datetime.datetime.strptime(request.GET["end_date"], "%Y-%m-%d").replace(tzinfo=pytz.UTC) now = datetime.datetime.now(pytz.UTC) if start_date > now or end_date > now or start_date > end_date: - raise RetirementStateError('Dates must be today or earlier, and start must be earlier than end.') + raise RetirementStateError("Dates must be today or earlier, and start must be earlier than end.") # Add a day to make sure we get all the way to 23:59:59.999, this is compared "lt" in the query # not "lte". end_date += datetime.timedelta(days=1) - state = request.GET['state'] + state = request.GET["state"] state_obj = RetirementState.objects.get(state_name=state) - retirements = UserRetirementStatus.objects.select_related( - 'user', 'current_state', 'last_state', 'user__profile' - ).filter( - current_state=state_obj, created__lt=end_date, created__gte=start_date - ).order_by( - 'id' + retirements = ( + UserRetirementStatus.objects.select_related("user", "current_state", "last_state", "user__profile") + .filter(current_state=state_obj, created__lt=end_date, created__gte=start_date) + .order_by("id") ) serializer = UserRetirementStatusSerializer(retirements, many=True) return Response(serializer.data) # This should only occur on the datetime conversion of the start / end dates. except ValueError as exc: - return Response(f'Invalid start or end date: {str(exc)}', status=status.HTTP_400_BAD_REQUEST) + return Response(f"Invalid start or end date: {str(exc)}", status=status.HTTP_400_BAD_REQUEST) except KeyError as exc: - return Response(f'Missing required parameter: {str(exc)}', - status=status.HTTP_400_BAD_REQUEST) + return Response(f"Missing required parameter: {str(exc)}", status=status.HTTP_400_BAD_REQUEST) except RetirementState.DoesNotExist: - return Response('Unknown retirement state.', status=status.HTTP_400_BAD_REQUEST) + return Response("Unknown retirement state.", status=status.HTTP_400_BAD_REQUEST) except RetirementStateError as exc: return Response(str(exc), status=status.HTTP_400_BAD_REQUEST) @@ -980,9 +966,9 @@ def retrieve(self, request, username): # pylint: disable=unused-argument """ try: user = get_potentially_retired_user_by_username(username) - retirement = UserRetirementStatus.objects.select_related( - 'user', 'current_state', 'last_state' - ).get(user=user) + retirement = UserRetirementStatus.objects.select_related("user", "current_state", "last_state").get( + user=user + ) serializer = UserRetirementStatusSerializer(instance=retirement) return Response(serializer.data) except (UserRetirementStatus.DoesNotExist, User.DoesNotExist): @@ -1008,7 +994,7 @@ def partial_update(self, request): The content type for this request is 'application/json'. """ try: - username = request.data['username'] + username = request.data["username"] retirements = UserRetirementStatus.objects.filter(original_username=username) # During a narrow window learners were able to re-use a username that had been retired if @@ -1049,20 +1035,19 @@ def cleanup(self, request): Deletes a batch of retirement requests by username. """ try: - usernames = request.data['usernames'] + usernames = request.data["usernames"] if not isinstance(usernames, list): - raise TypeError('Usernames should be an array.') + raise TypeError("Usernames should be an array.") - complete_state = RetirementState.objects.get(state_name='COMPLETE') + complete_state = RetirementState.objects.get(state_name="COMPLETE") retirements = UserRetirementStatus.objects.filter( - original_username__in=usernames, - current_state=complete_state + original_username__in=usernames, current_state=complete_state ) # Sanity check that they're all valid usernames in the right state if len(usernames) != len(retirements): - raise UserRetirementStatus.DoesNotExist('Not all usernames exist in the COMPLETE state.') + raise UserRetirementStatus.DoesNotExist("Not all usernames exist in the COMPLETE state.") retirements.delete() return Response(status=status.HTTP_204_NO_CONTENT) @@ -1076,7 +1061,11 @@ class LMSAccountRetirementView(ViewSet): """ Provides an API endpoint for retiring a user in the LMS. """ - permission_classes = (permissions.IsAuthenticated, CanRetireUser,) + + permission_classes = ( + permissions.IsAuthenticated, + CanRetireUser, + ) parser_classes = (JSONParser,) @request_requires_username @@ -1093,13 +1082,13 @@ def post(self, request): Retires the user with the given username in the LMS. """ - username = request.data['username'] + username = request.data["username"] try: retirement = UserRetirementStatus.get_retirement_for_retirement_action(username) RevisionPluginRevision.retire_user(retirement.user) ArticleRevision.retire_user(retirement.user) - PendingNameChange.delete_by_user_value(retirement.user, field='user') + PendingNameChange.delete_by_user_value(retirement.user, field="user") ManualEnrollmentAudit.retire_manual_enrollments(retirement.user, retirement.retired_email) CreditRequest.retire_user(retirement) @@ -1115,7 +1104,7 @@ def post(self, request): sender=self.__class__, email=retirement.original_email, new_email=retirement.retired_email, - user=retirement.user + user=retirement.user, ) except UserRetirementStatus.DoesNotExist: return Response(status=status.HTTP_404_NOT_FOUND) @@ -1131,7 +1120,11 @@ class AccountRetirementView(ViewSet): """ Provides API endpoint for retiring a user. """ - permission_classes = (permissions.IsAuthenticated, CanRetireUser,) + + permission_classes = ( + permissions.IsAuthenticated, + CanRetireUser, + ) parser_classes = (JSONParser,) @request_requires_username @@ -1148,7 +1141,7 @@ def post(self, request): Retires the user with the given username. This includes retiring this username, the associated email address, and any other PII associated with this user. """ - username = request.data['username'] + username = request.data["username"] try: retirement_status = UserRetirementStatus.get_retirement_for_retirement_action(username) @@ -1173,18 +1166,18 @@ def post(self, request): self.retire_entitlement_support_detail(user) # Retire misc. models that may contain PII of this user - PendingEmailChange.delete_by_user_value(user, field='user') - UserOrgTag.delete_by_user_value(user, field='user') + PendingEmailChange.delete_by_user_value(user, field="user") + UserOrgTag.delete_by_user_value(user, field="user") # Retire any objects linked to the user via their original email - CourseEnrollmentAllowed.delete_by_user_value(original_email, field='email') - UnregisteredLearnerCohortAssignments.delete_by_user_value(original_email, field='email') + CourseEnrollmentAllowed.delete_by_user_value(original_email, field="email") + UnregisteredLearnerCohortAssignments.delete_by_user_value(original_email, field="email") # This signal allows code in higher points of LMS to retire the user as necessary USER_RETIRE_LMS_CRITICAL.send(sender=self.__class__, user=user) - user.first_name = '' - user.last_name = '' + user.first_name = "" + user.last_name = "" user.is_active = False user.username = retired_username user.save() @@ -1227,24 +1220,20 @@ def retire_users_data_sharing_consent(username, retired_username): @staticmethod def retire_sapsf_data_transmission(user): # lint-amnesty, pylint: disable=missing-function-docstring for ent_user in EnterpriseCustomerUser.objects.filter(user_id=user.id): - for enrollment in EnterpriseCourseEnrollment.objects.filter( - enterprise_customer_user=ent_user - ): + for enrollment in EnterpriseCourseEnrollment.objects.filter(enterprise_customer_user=ent_user): audits = SapSuccessFactorsLearnerDataTransmissionAudit.objects.filter( enterprise_course_enrollment_id=enrollment.id ) - audits.update(sapsf_user_id='') + audits.update(sapsf_user_id="") @staticmethod def retire_degreed_data_transmission(user): # lint-amnesty, pylint: disable=missing-function-docstring for ent_user in EnterpriseCustomerUser.objects.filter(user_id=user.id): - for enrollment in EnterpriseCourseEnrollment.objects.filter( - enterprise_customer_user=ent_user - ): + for enrollment in EnterpriseCourseEnrollment.objects.filter(enterprise_customer_user=ent_user): audits = DegreedLearnerDataTransmissionAudit.objects.filter( enterprise_course_enrollment_id=enrollment.id ) - audits.update(degreed_user_email='') + audits.update(degreed_user_email="") @staticmethod def retire_user_from_pending_enterprise_customer_user(user, retired_email): @@ -1256,7 +1245,7 @@ def retire_entitlement_support_detail(user): Updates all CourseEntitleSupportDetail records for the given user to have an empty ``comments`` field. """ for entitlement in CourseEntitlement.objects.filter(user_id=user.id): - entitlement.courseentitlementsupportdetail_set.all().update(comments='') + entitlement.courseentitlementsupportdetail_set.all().update(comments="") @staticmethod def clear_pii_from_certificate_records(user): @@ -1279,6 +1268,7 @@ class UsernameReplacementView(APIView): This API will be called first, before calling the APIs in other services as this one handles the checks on the usernames provided. """ + permission_classes = (permissions.IsAuthenticated, CanReplaceUsername) def post(self, request): @@ -1320,16 +1310,16 @@ def post(self, request): # (model_name, column_name) MODELS_WITH_USERNAME = ( - ('auth.user', 'username'), - ('consent.DataSharingConsent', 'username'), - ('consent.HistoricalDataSharingConsent', 'username'), - ('credit.CreditEligibility', 'username'), - ('credit.CreditRequest', 'username'), - ('credit.CreditRequirementStatus', 'username'), - ('user_api.UserRetirementPartnerReportingStatus', 'original_username'), - ('user_api.UserRetirementStatus', 'original_username') + ("auth.user", "username"), + ("consent.DataSharingConsent", "username"), + ("consent.HistoricalDataSharingConsent", "username"), + ("credit.CreditEligibility", "username"), + ("credit.CreditRequest", "username"), + ("credit.CreditRequirementStatus", "username"), + ("user_api.UserRetirementPartnerReportingStatus", "original_username"), + ("user_api.UserRetirementStatus", "original_username"), ) - UNIQUE_SUFFIX_LENGTH = getattr(settings, 'SOCIAL_AUTH_UUID_LENGTH', 4) + UNIQUE_SUFFIX_LENGTH = getattr(settings, "SOCIAL_AUTH_UUID_LENGTH", 4) username_mappings = request.data.get("username_mappings") replacement_locations = self._load_models(MODELS_WITH_USERNAME) @@ -1344,9 +1334,7 @@ def post(self, request): desired_username = list(username_pair.values())[0] new_username = self._generate_unique_username(desired_username, suffix_length=UNIQUE_SUFFIX_LENGTH) successfully_replaced = self._replace_username_for_all_models( - current_username, - new_username, - replacement_locations + current_username, new_username, replacement_locations ) if successfully_replaced: successful_replacements.append({current_username: new_username}) @@ -1354,14 +1342,11 @@ def post(self, request): failed_replacements.append({current_username: new_username}) return Response( status=status.HTTP_200_OK, - data={ - "successful_replacements": successful_replacements, - "failed_replacements": failed_replacements - } + data={"successful_replacements": successful_replacements, "failed_replacements": failed_replacements}, ) def _load_models(self, models_with_fields): - """ Takes tuples that contain a model path and returns the list with a loaded version of the model """ + """Takes tuples that contain a model path and returns the list with a loaded version of the model""" try: replacement_locations = [(apps.get_model(model), column) for (model, column) in models_with_fields] except LookupError: @@ -1370,7 +1355,7 @@ def _load_models(self, models_with_fields): return replacement_locations def _has_valid_schema(self, post_data): - """ Verifies the data is a list of objects with a single key:value pair """ + """Verifies the data is a list of objects with a single key:value pair""" if not isinstance(post_data, list): return False for obj in post_data: @@ -1389,7 +1374,7 @@ def _generate_unique_username(self, desired_username, suffix_length=4): while True: if User.objects.filter(username=new_username).exists(): # adding a dash between user-supplied and system-generated values to avoid weird combinations - new_username = desired_username + '-' + username_suffix_generator(suffix_length) + new_username = desired_username + "-" + username_suffix_generator(suffix_length) else: break return new_username @@ -1404,10 +1389,8 @@ def _replace_username_for_all_models(self, current_username, new_username, repla try: with transaction.atomic(): num_rows_changed = 0 - for (model, column) in replacement_locations: - num_rows_changed += model.objects.filter( - **{column: current_username} - ).update( + for model, column in replacement_locations: + num_rows_changed += model.objects.filter(**{column: current_username}).update( **{column: new_username} ) except Exception as exc: # pylint: disable=broad-except @@ -1416,7 +1399,7 @@ def _replace_username_for_all_models(self, current_username, new_username, repla current_username, new_username, model.__class__.__name__, # Retrieves the model name that it failed on - exc + exc, ) return False if num_rows_changed == 0: From b30318af6a7e52b4401df7e74955b1c24df16eb3 Mon Sep 17 00:00:00 2001 From: michaelroytman Date: Mon, 12 Aug 2024 16:55:45 -0400 Subject: [PATCH 09/12] feat: add VerificationAttempt model to verify_student application This commits adds a VerificationAttempt model to store implementation and provider agnostic information about identity verification attempts in the platform. --- .../0001_extending_identity_verification.rst | 65 +++++++++++++++++++ .../migrations/0015_verificationattempt.py | 33 ++++++++++ lms/djangoapps/verify_student/models.py | 25 +++++++ lms/djangoapps/verify_student/statuses.py | 21 ++++++ 4 files changed, 144 insertions(+) create mode 100644 lms/djangoapps/verify_student/docs/decisions/0001_extending_identity_verification.rst create mode 100644 lms/djangoapps/verify_student/migrations/0015_verificationattempt.py create mode 100644 lms/djangoapps/verify_student/statuses.py diff --git a/lms/djangoapps/verify_student/docs/decisions/0001_extending_identity_verification.rst b/lms/djangoapps/verify_student/docs/decisions/0001_extending_identity_verification.rst new file mode 100644 index 000000000000..08735188fcdc --- /dev/null +++ b/lms/djangoapps/verify_student/docs/decisions/0001_extending_identity_verification.rst @@ -0,0 +1,65 @@ +0001. Extending Identity Verification +##################################### + +Status +****** + +**Accepted** *2024-08-26* + +Context +******* + +The backend implementation of identity verification (IDV) is in the `verify_student Django application`_. The +`verify_student Django application`_ also contains a frontend user experience for performing photo IDV via an +integration with Software Secure. There is also a `React-based implementation of this flow`_ in the +`frontend-app-account MFE`_, so the frontend user experience stored in the `verify_student Django application`_ is often +called the "legacy flow". + +The current architecture of the `verify_student Django application`_ requires that any additional implementations of IDV +are stored in the application. For example, the Software Secure integration is stored in this application even though +it is a custom integration that the Open edX community does not use. + +Different Open edX operators have different IDV needs. There is currently no way to add additional IDV implementations +to the platform without committing them to the core. The `verify_student Django application`_ needs enhanced +extensibility mechanisms to enable per-deployment integration of IDV implementations without modifying the core. + +Decision +******** + +* We will support the integration of additional implementations of IDV through the use of Python plugins into the + platform. +* We will add a ``VerificationAttempt`` model, which will store generic, implementation-agnostic information about an + IDV attempt. +* We will expose a simple Python API to write and update instances of the ``VerificationAttempt`` model. This will + enable plugins to publish information about their IDV attempts to the platform. +* The ``VerificationAttempt`` model will be integrated into the `verify_student Django application`_, particularly into + the `IDVerificationService`_. +* We will emit Open edX events for each status change of a ``VerificationAttempt``. +* We will add an Open edX filter hook to change the URL of the photo IDV frontend. + +Consequences +************ + +* It will become possible for Open edX operators to implement and integrate any additional forms of IDV necessary for + their deployment. +* The `verify_student Django application`_ will contain both concrete implementations of forms of IDV (i.e. manual, SSO, + Software Secure, etc.) and a generic, extensible implementation. The work to deprecate and remove the Software Secure + integration and to transition the other existing forms of IDV (i.e. manual and SSO) to Django plugins will occur + independently of the improvements to extensibility described in this decision. + +Rejected Alternatives +********************* + +We considered introducing a ``fetch_verification_attempts`` filter hook to allow plugins to expose additional +``VerificationAttempts`` to the platform in lieu of an additional model. However, doing database queries via filter +hooks can cause unpredictable performance problems, and this has been a pain point for Open edX. + +References +********** +`[Proposal] Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona `_ +`Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona `_ + +.. _frontend-app-account MFE: https://github.com/openedx/frontend-app-account +.. _IDVerificationService: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/verify_student/services.py#L55 +.. _React-based implementation of this flow: https://github.com/openedx/frontend-app-account/tree/master/src/id-verification +.. _verify_student Django application: https://github.com/openedx/edx-platform/tree/master/lms/djangoapps/verify_student diff --git a/lms/djangoapps/verify_student/migrations/0015_verificationattempt.py b/lms/djangoapps/verify_student/migrations/0015_verificationattempt.py new file mode 100644 index 000000000000..3f01047f9f51 --- /dev/null +++ b/lms/djangoapps/verify_student/migrations/0015_verificationattempt.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.15 on 2024-08-26 14:05 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('verify_student', '0014_remove_softwaresecurephotoverification_expiry_date'), + ] + + operations = [ + migrations.CreateModel( + name='VerificationAttempt', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('name', models.CharField(blank=True, max_length=255)), + ('status', models.CharField(choices=[('created', 'created'), ('pending', 'pending'), ('approved', 'approved'), ('denied', 'denied')], max_length=64)), + ('expiration_datetime', models.DateTimeField(blank=True, null=True)), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'abstract': False, + }, + ), + ] diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index f7750a4cd662..903d80bf9245 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -31,6 +31,7 @@ from django.utils.translation import gettext_lazy from model_utils import Choices from model_utils.models import StatusModel, TimeStampedModel +from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus from opaque_keys.edx.django.models import CourseKeyField from lms.djangoapps.verify_student.ssencrypt import ( @@ -1189,3 +1190,27 @@ class Meta: def __str__(self): return str(self.arguments) + + +class VerificationAttempt(TimeStampedModel): + """ + The model represents impelementation-agnostic information about identity verification (IDV) attempts. + + Plugins that implement forms of IDV can store information about IDV attempts in this model for use across + the platform. + """ + user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) + name = models.CharField(blank=True, max_length=255) + + STATUS_CHOICES = [ + VerificationAttemptStatus.created, + VerificationAttemptStatus.pending, + VerificationAttemptStatus.approved, + VerificationAttemptStatus.denied, + ] + status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) + + expiration_datetime = models.DateTimeField( + null=True, + blank=True, + ) diff --git a/lms/djangoapps/verify_student/statuses.py b/lms/djangoapps/verify_student/statuses.py new file mode 100644 index 000000000000..b55a9042e0f6 --- /dev/null +++ b/lms/djangoapps/verify_student/statuses.py @@ -0,0 +1,21 @@ +""" +Status enums for verify_student. +""" + + +class VerificationAttemptStatus: + """This class describes valid statuses for a verification attempt to be in.""" + + # This is the initial state of a verification attempt, before a learner has started IDV. + created = "created" + + # A verification attempt is pending when it has been started but has not yet been completed. + pending = "pending" + + # A verification attempt is approved when it has been approved by some mechanism (e.g. automatic review, manual + # review, etc). + approved = "approved" + + # A verification attempt is denied when it has been denied by some mechanism (e.g. automatic review, manual review, + # etc). + denied = "denied" From a9d6d4b20d0e15280cb312e9fd98ad3956bbc0e8 Mon Sep 17 00:00:00 2001 From: MueezKhan246 <93375917+MueezKhan246@users.noreply.github.com> Date: Mon, 26 Aug 2024 17:40:07 +0000 Subject: [PATCH 10/12] feat: Upgrade Python dependency edx-enterprise added encrypted columns for user credentials for SAP config Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 74263b5f7141..b7c7bebc71d6 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -26,7 +26,7 @@ celery>=5.2.2,<6.0.0 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==4.23.9 +edx-enterprise==4.23.13 # Stay on LTS version, remove once this is added to common constraint Django<5.0 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 27de7847f284..1ed691bf5a51 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -467,7 +467,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.23.9 +edx-enterprise==4.23.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 0bdc8144ee71..0f88bad4b5b6 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -741,7 +741,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.23.9 +edx-enterprise==4.23.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 91b30d81df6d..663e09b3d685 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -547,7 +547,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.23.9 +edx-enterprise==4.23.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2092c9354834..e8ed020e5128 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -571,7 +571,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.23.9 +edx-enterprise==4.23.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 275d4d989fd0a6ba5033448eca3228c42d572a42 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Tue, 27 Aug 2024 14:23:59 +0500 Subject: [PATCH 11/12] feat: show_student_extensions upgrading api to drf compatible ( 9th ) (#35148) * feat: upgrading simple api to drf compatible. --- lms/djangoapps/instructor/views/api.py | 54 +++++++++++++++---- lms/djangoapps/instructor/views/api_urls.py | 2 +- lms/djangoapps/instructor/views/serializer.py | 18 +++++++ 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 7ca1e70467b0..1aa40b5e3376 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -105,7 +105,9 @@ from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import ReportStore -from lms.djangoapps.instructor.views.serializer import RoleNameSerializer, UserSerializer, AccessSerializer +from lms.djangoapps.instructor.views.serializer import ( + AccessSerializer, RoleNameSerializer, ShowStudentExtensionSerializer, UserSerializer +) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted from openedx.core.djangoapps.course_groups.models import CourseUserGroup @@ -2979,20 +2981,50 @@ def show_unit_extensions(request, course_id): return JsonResponse(dump_block_extensions(course, unit)) -@handle_dashboard_error -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.GIVE_STUDENT_EXTENSION) -@require_post_params('student') -def show_student_extensions(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ShowStudentExtensions(APIView): """ Shows all of the due date extensions granted to a particular student in a particular course. """ - student = require_student_from_identifier(request.POST.get('student')) - course = get_course_by_id(CourseKey.from_string(course_id)) - return JsonResponse(dump_student_extensions(course, student)) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + serializer_class = ShowStudentExtensionSerializer + permission_name = permissions.GIVE_STUDENT_EXTENSION + + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + Handles POST requests to retrieve due date extensions for a specific student + within a specified course. + + Parameters: + - `request`: The HTTP request object containing user-submitted data. + - `course_id`: The ID of the course for which the extensions are being queried. + + Data expected in the request: + - `student`: A required field containing the identifier of the student for whom + the due date extensions are being retrieved. This data is extracted from the + request body. + + Returns: + - A JSON response containing the details of the due date extensions granted to + the specified student in the specified course. + """ + data = { + 'student': request.data.get('student') + } + serializer_data = self.serializer_class(data=data) + + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) + + student = serializer_data.validated_data.get('student') + if not student: + response_payload = f'Could not find student matching identifier: {request.data.get("student")}' + return JsonResponse({'error': response_payload}, status=400) + + course = get_course_by_id(CourseKey.from_string(course_id)) + return Response(dump_student_extensions(course, student)) def _split_input_list(str_list): diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index c0e12e46756d..18da0b63b218 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -53,7 +53,7 @@ path('change_due_date', api.change_due_date, name='change_due_date'), path('reset_due_date', api.reset_due_date, name='reset_due_date'), path('show_unit_extensions', api.show_unit_extensions, name='show_unit_extensions'), - path('show_student_extensions', api.show_student_extensions, name='show_student_extensions'), + path('show_student_extensions', api.ShowStudentExtensions.as_view(), name='show_student_extensions'), # proctored exam downloads... path('get_proctored_exam_results', api.get_proctored_exam_results, name='get_proctored_exam_results'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 463f05ad45b8..0697bed6832d 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -59,3 +59,21 @@ def validate_unique_student_identifier(self, value): return None return user + + +class ShowStudentExtensionSerializer(serializers.Serializer): + """ + Serializer for validating and processing the student identifier. + """ + student = serializers.CharField(write_only=True, required=True) + + def validate_student(self, value): + """ + Validate that the student corresponds to an existing user. + """ + try: + user = get_student_from_identifier(value) + except User.DoesNotExist: + return None + + return user From f0d8d5262c03ff5d8ee891fed59bb06678d7a002 Mon Sep 17 00:00:00 2001 From: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com> Date: Tue, 27 Aug 2024 09:06:28 -0400 Subject: [PATCH 12/12] fix: add aria-current attributes to video captions (#35371) This PR fixes the accessibility issues associated with only visually treating the current transcript line. The current implementation for the transcript panel bolds the text that is actively being spoken or skipped to. However, there is no aria attribute present to convey this change to assistive technology. This change impacts learners and course authors. --- xmodule/js/src/tabs/tabs-aggregator.js | 4 ++-- xmodule/js/src/video/09_video_caption.js | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/xmodule/js/src/tabs/tabs-aggregator.js b/xmodule/js/src/tabs/tabs-aggregator.js index 83baca4cf6e9..da982b6afc9d 100644 --- a/xmodule/js/src/tabs/tabs-aggregator.js +++ b/xmodule/js/src/tabs/tabs-aggregator.js @@ -63,8 +63,8 @@ if ($.isFunction(onSwitchFunction)) { onSwitchFunction(); } - this.$tabs.removeClass('current'); - $currentTarget.addClass('current'); + this.$tabs.attr('aria-current', 'false').removeClass('current'); + $currentTarget.attr('aria-current', 'true').addClass('current'); /* Tabs are implemeted like anchors. Therefore we can use hash to find diff --git a/xmodule/js/src/video/09_video_caption.js b/xmodule/js/src/video/09_video_caption.js index 37e3923067d5..f5db26514b64 100644 --- a/xmodule/js/src/video/09_video_caption.js +++ b/xmodule/js/src/video/09_video_caption.js @@ -1096,12 +1096,13 @@ if (typeof this.currentIndex !== 'undefined') { this.subtitlesEl .find('li.current') + .attr('aria-current', 'false') .removeClass('current'); - } - + } this.subtitlesEl .find("span[data-index='" + newIndex + "']") .parent() + .attr('aria-current', 'true') .addClass('current'); this.currentIndex = newIndex;