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 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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