From ea3dc9cad6aad0138bed352867a38528e17bf690 Mon Sep 17 00:00:00 2001 From: michaelroytman Date: Thu, 16 Dec 2021 13:00:03 -0500 Subject: [PATCH] fix: Do Not Prevent Learner From Changing Name With Non-Verified Enrollment/Non-Passable Certificate The _does_name_change_require_verification(user_profile, old_name, new_name) method of the accounts user_api determines whether a learner can change their name from old_name to new_name. Originally, it delegated solely to the NameChangeValidator class of the edx-name-affirmation API, which ran a set of checks against the name change. One of said checks was asserting that learners with one or more certificates could not change their name without completing IDV. This pull request changes this behavior. Learners may have certificates that are not in a passable status (e.g. "unverified"). We only want to require IDV for name changes for learners that have passing statuses. The existing code prevented learners from changing their name if they had any certificates at all, irrespective of the certificate status. This change only considers certificates in a passable status. Additionally, learners may have certificates and also not be enrolled in any "verified" seats. For example, despite edX no longer offering "honor" seats, learners may have enrollments in "honor" modes, which grant certificates but are not considered "verified" enrollment modes. IDV requires that a learner be enrolled in a "verified" seat in order to complete IDV. Prior to this change, learners that were navigated to IDV to validate a name change were unable to complete IDV. This change introduce a check that a learner is in a "verified" mode in addition to using the NameChangeValidator. This prevents the account MFE from navigating an IDV-ineligible learner to IDV. MST-1254: https://openedx.atlassian.net/browse/MST-1254 --- openedx/core/djangoapps/enrollments/api.py | 53 +++++++++++++++++++ .../djangoapps/enrollments/tests/test_api.py | 25 +++++++++ .../core/djangoapps/user_api/accounts/api.py | 25 +++++++-- .../user_api/accounts/tests/test_api.py | 27 ++++++++-- 4 files changed, 121 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/enrollments/api.py b/openedx/core/djangoapps/enrollments/api.py index dd5d55a9a4..6f3b22d7dd 100644 --- a/openedx/core/djangoapps/enrollments/api.py +++ b/openedx/core/djangoapps/enrollments/api.py @@ -21,6 +21,59 @@ log = logging.getLogger(__name__) DEFAULT_DATA_API = 'openedx.core.djangoapps.enrollments.data' +def get_verified_enrollments(username, include_inactive=False): + """Retrieves all the courses in which user is enrolled in a verified mode. + + Takes a user and retrieves all relative enrollments in which the learner is enrolled in a verified mode. + Includes information regarding how the user is enrolled + in the the course. + + Args: + username: The username of the user we want to retrieve course enrollment information for. + include_inactive (bool): Determines whether inactive enrollments will be included + + Returns: + A list of enrollment information for the given user. + + Examples: + >>> get_verified_enrollments("Bob") + [ + { + "created": "2014-10-25T20:18:00Z", + "mode": "verified", + "is_active": True, + "user": "Bob", + "course_details": { + "course_id": "edX/edX-Insider/2014T2", + "course_name": "edX Insider Course", + "enrollment_end": "2014-12-20T20:18:00Z", + "enrollment_start": "2014-10-15T20:18:00Z", + "course_start": "2015-02-03T00:00:00Z", + "course_end": "2015-05-06T00:00:00Z", + "course_modes": [ + { + "slug": "honor", + "name": "Honor Code Certificate", + "min_price": 0, + "suggested_prices": "", + "currency": "usd", + "expiration_datetime": null, + "description": null, + "sku": null, + "bulk_sku": null + } + ], + "invite_only": True + } + } + ] + + """ + enrollments = get_enrollments(username, include_inactive) + enrollments = filter(lambda enrollment: CourseMode.is_verified_slug(enrollment['mode']), enrollments) + return list(enrollments) + + def get_enrollments(username, include_inactive=False): """Retrieves all the courses a user is enrolled in. diff --git a/openedx/core/djangoapps/enrollments/tests/test_api.py b/openedx/core/djangoapps/enrollments/tests/test_api.py index e3a95df6fd..3f00318e09 100644 --- a/openedx/core/djangoapps/enrollments/tests/test_api.py +++ b/openedx/core/djangoapps/enrollments/tests/test_api.py @@ -165,6 +165,31 @@ class EnrollmentTest(CacheIsolationTestCase): for result_enrollment in result: assert result_enrollment['course']['course_id'] in [enrollment['course_id'] for enrollment in enrollments] + @ddt.data( + # Simple test of honor and verified. + ([ + {'course_id': 'the/first/course', 'course_modes': [], 'mode': 'honor'}, + {'course_id': 'the/second/course', 'course_modes': ['honor', 'verified'], 'mode': 'verified'} + ], 1), + + # No enrollments + ([], 0), + + # One Enrollment + ([ + {'course_id': 'the/third/course', 'course_modes': ['honor', 'verified', 'audit'], 'mode': 'audit'} + ], 0), + ) + @ddt.unpack + def test_get_verified_enrollments(self, enrollments, num_verified_enrollments): + for enrollment in enrollments: + fake_data_api.add_course(enrollment['course_id'], course_modes=enrollment['course_modes']) + api.add_enrollment(self.USERNAME, enrollment['course_id'], enrollment['mode']) + result = api.get_verified_enrollments(self.USERNAME) + assert num_verified_enrollments == len(result) + for result_enrollment in result: + assert result_enrollment['course']['course_id'] in [enrollment['course_id'] for enrollment in enrollments] + def test_update_enrollment(self): # Add fake course enrollment information to the fake data API fake_data_api.add_course(self.COURSE_ID, course_modes=['honor', 'verified', 'audit']) diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 7510da5cdd..2412db8bfe 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -24,7 +24,9 @@ from common.djangoapps.student.models import ( from common.djangoapps.util.model_utils import emit_settings_changed_event from common.djangoapps.util.password_policy_validators import validate_password from lms.djangoapps.certificates.api import get_certificates_for_user +from lms.djangoapps.certificates.data import CertificateStatuses +from openedx.core.djangoapps.enrollments.api import get_verified_enrollments from openedx.core.djangoapps.user_api import accounts, errors, helpers from openedx.core.djangoapps.user_api.errors import ( AccountUpdateError, @@ -270,18 +272,31 @@ def _validate_name_change(user_profile, data, field_errors): def _does_name_change_require_verification(user_profile, old_name, new_name): """ - If name change requires verification, do not update it through this API. + If name change requires ID verification, do not update it through this API. """ - profile_meta = user_profile.get_meta() old_names_list = profile_meta['old_names'] if 'old_names' in profile_meta else [] user = user_profile.user - num_certs = len(get_certificates_for_user(user.username)) - validator = NameChangeValidator(old_names_list, num_certs, old_name, new_name) + # We only want to validate on a list of passing certificates for the learner. A learner may have + # a certificate in a non-passing status, and we do not have to require ID verification based on certificates + # that are not passing. + passing_certs = filter( + lambda cert: CertificateStatuses.is_passing_status(cert["status"]), + get_certificates_for_user(user.username) + ) + num_passing_certs = len(list(passing_certs)) - return not validator.validate() + # We check whether the learner has active verified enrollments because we do not want to + # require the learner to perform ID verification if the learner is not enrolled in a verified mode + # in any courses. The learner will not be able to complete ID verification without being enrolled in + # at least one seat. + has_verified_enrollments = len(get_verified_enrollments(user.username)) > 0 + + validator = NameChangeValidator(old_names_list, num_passing_certs, old_name, new_name) + + return not validator.validate() and has_verified_enrollments def _get_old_language_proficiencies_if_updating(user_profile, data): diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 581a414227..98262ab75b 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -29,6 +29,7 @@ from common.djangoapps.student.tests.factories import UserFactory from common.djangoapps.student.tests.tests import UserSettingsEventTestMixin from common.djangoapps.student.views.management import activate_secondary_email +from lms.djangoapps.certificates.data import CertificateStatuses from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY from openedx.core.djangoapps.user_api.accounts.api import ( @@ -384,15 +385,18 @@ class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, CreateAc with patch( 'openedx.core.djangoapps.user_api.accounts.api.get_certificates_for_user', - return_value=['mock_certificate'] + return_value=[{'status': CertificateStatuses.downloadable}] ): update_account_settings(self.user, {'name': account_settings['name']}) # The name should not be added to profile metadata updated_meta = user_profile.get_meta() self.assertEqual(meta, updated_meta) - @patch('edx_name_affirmation.name_change_validator.NameChangeValidator', Mock()) @patch('edx_name_affirmation.name_change_validator.NameChangeValidator.validate', Mock(return_value=False)) + @patch('openedx.core.djangoapps.user_api.accounts.api.get_certificates_for_user', + Mock(return_value=[{'status': CertificateStatuses.downloadable}])) + @patch('openedx.core.djangoapps.user_api.accounts.api.get_verified_enrollments', + Mock(return_value=[{'name': 'Bob'}])) def test_name_update_requires_idv(self): """ Test that a name change is blocked through this API if it requires ID verification. @@ -409,11 +413,26 @@ class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, CreateAc @patch('edx_name_affirmation.name_change_validator.NameChangeValidator', Mock()) @patch('edx_name_affirmation.name_change_validator.NameChangeValidator.validate', Mock(return_value=True)) - def test_name_update_does_not_require_idv(self): + @ddt.data( + (True, False), + (False, True), + (False, False) + ) + @ddt.unpack + def test_name_update_does_not_require_idv(self, has_passable_cert, enrolled_in_verified_mode): """ Test that the user can change their name if change does not require IDV. """ - update_account_settings(self.user, {'name': 'New Name'}) + with patch('openedx.core.djangoapps.user_api.accounts.api.get_certificates_for_user') as mock_get_certs,\ + patch('openedx.core.djangoapps.user_api.accounts.api.get_verified_enrollments') as \ + mock_get_verified_enrollments: + mock_get_certs.return_value = ( + [{'status': CertificateStatuses.downloadable}] if + has_passable_cert else + [{'status': CertificateStatuses.unverified}] + ) + mock_get_verified_enrollments.return_value = [{'name': 'Bob'}] if enrolled_in_verified_mode else [] + update_account_settings(self.user, {'name': 'New Name'}) account_settings = get_account_settings(self.default_request)[0] assert account_settings['name'] == 'New Name'