Merge pull request #28546 from edx/bseverino/pending-name-change

[MST-803] Update account API to allow pending name changes
This commit is contained in:
Bianca Severino
2021-09-02 14:45:00 -04:00
committed by GitHub
12 changed files with 355 additions and 22 deletions

View File

@@ -931,7 +931,7 @@ class Registration(models.Model):
class PendingNameChange(DeletableByUserValue, models.Model):
"""
This model keeps track of pending requested changes to a user's email address.
This model keeps track of pending requested changes to a user's name.
.. pii: Contains new_name, retired in LMSAccountRetirementView
.. pii_types: name

View File

@@ -1,8 +1,11 @@
"""
Provides Python APIs exposed from Student models.
"""
import datetime
import logging
from pytz import UTC
from common.djangoapps.student.models import CourseAccessRole as _CourseAccessRole
from common.djangoapps.student.models import CourseEnrollment as _CourseEnrollment
from common.djangoapps.student.models import ManualEnrollmentAudit as _ManualEnrollmentAudit
@@ -16,6 +19,7 @@ from common.djangoapps.student.models import (
ALLOWEDTOENROLL_TO_UNENROLLED as _ALLOWEDTOENROLL_TO_UNENROLLED,
DEFAULT_TRANSITION_STATE as _DEFAULT_TRANSITION_STATE,
)
from common.djangoapps.student.models import PendingNameChange as _PendingNameChange
from common.djangoapps.student.models import UserProfile as _UserProfile
# This is done so that if these strings change within the app, we can keep exported constants the same
@@ -103,3 +107,50 @@ def get_course_access_role(user, org, course_id, role):
})
return None
return course_access_role
def do_name_change_request(user, new_name, rationale):
"""
Create a name change request. This either updates the user's current PendingNameChange, or creates
a new one if it doesn't exist. Returns the PendingNameChange object and a boolean describing whether
or not a new one was created.
"""
user_profile = _UserProfile.objects.get(user=user)
if user_profile.name == new_name:
log_msg = (
'user_id={user_id} requested a name change, but the requested name is the same as'
'their current profile name. Not taking any action.'.format(user_id=user.id)
)
log.warning(log_msg)
return None, False
pending_name_change, created = _PendingNameChange.objects.update_or_create(
user=user,
defaults={
'new_name': new_name,
'rationale': rationale
}
)
return pending_name_change, created
def confirm_name_change(user, pending_name_change):
"""
Confirm a pending name change. This updates the user's profile name and deletes the
PendingNameChange object.
"""
user_profile = _UserProfile.objects.get(user=user)
# Store old name in profile metadata
meta = user_profile.get_meta()
if 'old_names' not in meta:
meta['old_names'] = []
meta['old_names'].append(
[user_profile.name, pending_name_change.rationale, datetime.datetime.now(UTC).isoformat()]
)
user_profile.set_meta(meta)
user_profile.name = pending_name_change.new_name
user_profile.save()
pending_name_change.delete()

View File

@@ -9,10 +9,18 @@ from django.contrib.auth import get_user_model
from django.db import IntegrityError
from django.db.models.signals import post_save, pre_save
from django.dispatch import receiver
from edx_name_affirmation.signals import VERIFIED_NAME_APPROVED
from lms.djangoapps.courseware.toggles import courseware_mfe_progress_milestones_are_active
from common.djangoapps.student.helpers import EMAIL_EXISTS_MSG_FMT, USERNAME_EXISTS_MSG_FMT, AccountValidationError
from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentCelebration, is_email_retired, is_username_retired # lint-amnesty, pylint: disable=line-too-long
from common.djangoapps.student.models import (
CourseEnrollment,
CourseEnrollmentCelebration,
PendingNameChange,
is_email_retired,
is_username_retired
)
from common.djangoapps.student.models_api import confirm_name_change
@receiver(pre_save, sender=get_user_model())
@@ -70,3 +78,16 @@ def create_course_enrollment_celebration(sender, instance, created, **kwargs):
except IntegrityError:
# A celebration object was already created. Shouldn't happen, but ignore it if it does.
pass
@receiver(VERIFIED_NAME_APPROVED)
def listen_for_verified_name_approved(sender, user_id, profile_name, **kwargs):
"""
If the user has a pending name change that corresponds to an approved verified name, confirm it.
"""
user = get_user_model().objects.get(id=user_id)
try:
pending_name_change = PendingNameChange.objects.get(user=user, new_name=profile_name)
confirm_name_change(user, pending_name_change)
except PendingNameChange.DoesNotExist:
pass

View File

@@ -29,7 +29,7 @@ from common.djangoapps.student.models import (
UserCelebration,
UserProfile
)
from common.djangoapps.student.models_api import get_name
from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_name
from common.djangoapps.student.tests.factories import AccountRecoveryFactory, CourseEnrollmentFactory, UserFactory
from lms.djangoapps.courseware.models import DynamicUpgradeDeadlineConfiguration
from lms.djangoapps.courseware.toggles import (
@@ -467,21 +467,56 @@ class PendingNameChangeTests(SharedModuleStoreTestCase):
super().setUpClass()
cls.user = UserFactory()
cls.user2 = UserFactory()
cls.name = cls.user.profile.name
cls.new_name = 'New Name'
cls.updated_name = 'Updated Name'
cls.rationale = 'Testing name change'
def setUp(self): # lint-amnesty, pylint: disable=super-method-not-called
self.name_change, _ = PendingNameChange.objects.get_or_create(
user=self.user,
new_name='New Name PII',
rationale='for testing!'
)
assert 1 == len(PendingNameChange.objects.all())
def test_do_name_change_request(self):
"""
Test basic name change request functionality.
"""
do_name_change_request(self.user, self.new_name, self.rationale)
self.assertEqual(PendingNameChange.objects.count(), 1)
def test_same_name(self):
"""
Test that attempting a name change with the same name as the user's current profile
name will not result in a new pending name change request.
"""
pending_name_change = do_name_change_request(self.user, self.name, self.rationale)[0]
self.assertIsNone(pending_name_change)
def test_update_name_change(self):
"""
Test that if a user already has a name change request, creating another request will
update the current one.
"""
do_name_change_request(self.user, self.new_name, self.rationale)
do_name_change_request(self.user, self.updated_name, self.rationale)
self.assertEqual(PendingNameChange.objects.count(), 1)
pending_name_change = PendingNameChange.objects.get(user=self.user)
self.assertEqual(pending_name_change.new_name, self.updated_name)
def test_confirm_name_change(self):
"""
Test that confirming a name change request updates the user's profile name and deletes
the request.
"""
pending_name_change = do_name_change_request(self.user, self.new_name, self.rationale)[0]
confirm_name_change(self.user, pending_name_change)
user_profile = UserProfile.objects.get(user=self.user)
self.assertEqual(user_profile.name, self.new_name)
self.assertEqual(PendingNameChange.objects.count(), 0)
def test_delete_by_user_removes_pending_name_change(self):
do_name_change_request(self.user, self.new_name, self.rationale)
record_was_deleted = PendingNameChange.delete_by_user_value(self.user, field='user')
assert record_was_deleted
assert 0 == len(PendingNameChange.objects.all())
def test_delete_by_user_no_effect_for_user_with_no_name_change(self):
do_name_change_request(self.user, self.new_name, self.rationale)
record_was_deleted = PendingNameChange.delete_by_user_value(self.user2, field='user')
assert not record_was_deleted
assert 1 == len(PendingNameChange.objects.all())

View File

@@ -1,9 +1,18 @@
""" Tests for student signal receivers. """
from edx_name_affirmation.signals import VERIFIED_NAME_APPROVED
from edx_toggles.toggles.testutils import override_waffle_flag
from lms.djangoapps.courseware.toggles import COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES
from common.djangoapps.student.models import CourseEnrollmentCelebration
from common.djangoapps.student.tests.factories import CourseEnrollmentFactory
from common.djangoapps.student.models import (
CourseEnrollmentCelebration,
PendingNameChange,
UserProfile
)
from common.djangoapps.student.tests.factories import (
CourseEnrollmentFactory,
UserFactory,
UserProfileFactory
)
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
@@ -33,3 +42,23 @@ class ReceiversTest(SharedModuleStoreTestCase):
""" Test we don't make a celebration if the MFE redirect waffle flag is off """
CourseEnrollmentFactory()
assert CourseEnrollmentCelebration.objects.count() == 0
def test_listen_for_verified_name_approved(self):
"""
Test that profile name is updated when a pending name change is approved
"""
user = UserFactory(email='email@test.com', username='jdoe')
UserProfileFactory(user=user)
new_name = 'John Doe'
PendingNameChange.objects.create(user=user, new_name=new_name)
assert PendingNameChange.objects.count() == 1
# Send a VERIFIED_NAME_APPROVED signal where the profile name matches the name
# change request
VERIFIED_NAME_APPROVED.send(sender=None, user_id=user.id, profile_name=new_name)
# Assert that the pending name change was deleted and the profile name was updated
assert PendingNameChange.objects.count() == 0
profile = UserProfile.objects.get(user=user)
assert profile.name == new_name

View File

@@ -4001,6 +4001,7 @@ ACCOUNT_VISIBILITY_CONFIGURATION["admin_fields"] = (
"year_of_birth",
"phone_number",
"activation_key",
"pending_name_change",
"is_verified_name_enabled",
]
)

View File

@@ -11,6 +11,7 @@ from django.core.exceptions import ObjectDoesNotExist
from django.core.validators import ValidationError, validate_email
from django.utils.translation import override as override_language
from django.utils.translation import ugettext as _
from edx_name_affirmation.toggles import is_verified_name_enabled
from pytz import UTC
from common.djangoapps.student import views as student_views
from common.djangoapps.student.models import (
@@ -22,6 +23,7 @@ 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 openedx.core.djangoapps.user_api import accounts, errors, helpers
from openedx.core.djangoapps.user_api.errors import (
@@ -240,6 +242,7 @@ def _validate_name_change(user_profile, data, field_errors):
return None
old_name = user_profile.name
try:
validate_name(data['name'])
except ValidationError as err:
@@ -249,9 +252,28 @@ def _validate_name_change(user_profile, data, field_errors):
}
return None
if _does_name_change_require_verification(user_profile.user, old_name, data['name']):
err_msg = 'This name change requires ID verification.'
field_errors['name'] = {
'developer_message': err_msg,
'user_message': err_msg
}
return None
return old_name
def _does_name_change_require_verification(user, old_name, new_name):
"""
If name change requires verification, do not update it through this API.
"""
return (
is_verified_name_enabled()
and old_name != new_name
and len(get_certificates_for_user(user.username)) > 0
)
def _get_old_language_proficiencies_if_updating(user_profile, data):
if "language_proficiencies" in data:
return list(user_profile.language_proficiencies.values('code'))

View File

@@ -15,14 +15,20 @@ from rest_framework import serializers
from edx_name_affirmation.toggles import is_verified_name_enabled
from common.djangoapps.student.models import UserPasswordToggleHistory
from common.djangoapps.student.models import (
LanguageProficiency,
PendingNameChange,
SocialLink,
UserPasswordToggleHistory,
UserProfile
)
from lms.djangoapps.badges.utils import badges_enabled
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.user_api import errors
from openedx.core.djangoapps.user_api.accounts.utils import is_secondary_email_feature_enabled
from openedx.core.djangoapps.user_api.models import RetirementState, UserPreference, UserRetirementStatus
from openedx.core.djangoapps.user_api.serializers import ReadOnlyFieldsSerializerMixin
from common.djangoapps.student.models import LanguageProficiency, SocialLink, UserProfile
from openedx.core.djangoapps.user_authn.views.registration_form import contains_html, contains_url
from . import (
ACCOUNT_VISIBILITY_PREF_KEY,
@@ -163,6 +169,7 @@ class UserReadOnlySerializer(serializers.Serializer): # lint-amnesty, pylint: d
"social_links": None,
"extended_profile_fields": None,
"phone_number": None,
"pending_name_change": None,
"is_verified_name_enabled": is_verified_name_enabled(),
}
@@ -196,6 +203,12 @@ class UserReadOnlySerializer(serializers.Serializer): # lint-amnesty, pylint: d
}
)
try:
pending_name_change = PendingNameChange.objects.get(user=user)
data.update({"pending_name_change": pending_name_change.new_name})
except PendingNameChange.DoesNotExist:
pass
if is_secondary_email_feature_enabled():
data.update(
{
@@ -528,6 +541,23 @@ class UserRetirementPartnerReportSerializer(serializers.Serializer):
pass
class PendingNameChangeSerializer(serializers.Serializer): # lint-amnesty, pylint: disable=abstract-method
"""
Serialize the PendingNameChange model
"""
new_name = serializers.CharField()
class Meta:
model = PendingNameChange
fields = ('new_name',)
def validate_new_name(self, new_name):
if contains_html(new_name):
raise serializers.ValidationError('Name cannot contain the following characters: < >')
if contains_url(new_name):
raise serializers.ValidationError('Name cannot contain a URL')
def get_extended_profile(user_profile):
"""
Returns the extended user profile fields stored in user_profile.meta

View File

@@ -16,6 +16,8 @@ from django.http import HttpResponse
from django.test import TestCase
from django.test.client import RequestFactory
from django.urls import reverse
from edx_name_affirmation.toggles import VERIFIED_NAME_FLAG
from edx_toggles.toggles.testutils import override_waffle_flag
from social_django.models import UserSocialAuth
from common.djangoapps.student.models import (
AccountRecovery,
@@ -361,6 +363,42 @@ class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, CreateAc
assert 'Valid e-mail address required.' in field_errors['email']['developer_message']
assert 'Full Name cannot contain the following characters: < >' in field_errors['name']['user_message']
@override_waffle_flag(VERIFIED_NAME_FLAG, active=True)
@patch(
'openedx.core.djangoapps.user_api.accounts.api.get_certificates_for_user',
Mock(return_value=['mock certificate'])
)
def test_name_update_requires_idv(self):
"""
Test that a name change is blocked through this API if it requires ID verification.
In this case, the user has at least one certificate.
"""
update = {'name': 'New Name'}
with pytest.raises(AccountValidationError) as context_manager:
update_account_settings(self.user, update)
field_errors = context_manager.value.field_errors
assert len(field_errors) == 1
assert field_errors['name']['developer_message'] == 'This name change requires ID verification.'
account_settings = get_account_settings(self.default_request)[0]
assert account_settings['name'] != 'New Name'
@override_waffle_flag(VERIFIED_NAME_FLAG, active=True)
@patch(
'openedx.core.djangoapps.user_api.accounts.api.get_certificates_for_user',
Mock(return_value=[])
)
def test_name_update_does_not_require_idv(self):
"""
Test that the user can change their name freely if it does not require verification.
"""
update = {'name': 'New Name'}
update_account_settings(self.user, update)
account_settings = get_account_settings(self.default_request)[0]
assert account_settings['name'] == 'New Name'
@patch('django.core.mail.EmailMultiAlternatives.send')
@patch('common.djangoapps.student.views.management.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) # lint-amnesty, pylint: disable=line-too-long
def test_update_sending_email_fails(self, send_mail):
@@ -555,6 +593,7 @@ class AccountSettingsOnCreationTest(CreateAccountMixin, TestCase):
'time_zone': None,
'course_certificates': None,
'phone_number': None,
'pending_name_change': None,
'is_verified_name_enabled': False,
}

View File

@@ -60,6 +60,16 @@ class UserAPITestCase(APITestCase):
client.login(username=user.username, password=TEST_PASSWORD)
return client
def send_post(self, client, json_data, content_type='application/json', expected_status=201):
"""
Helper method for sending a post to the server, defaulting to application/json content_type.
Verifies the expected status and returns the response.
"""
# pylint: disable=no-member
response = client.post(self.url, data=json.dumps(json_data), content_type=content_type)
assert expected_status == response.status_code
return response
def send_patch(self, client, json_data, content_type="application/merge-patch+json", expected_status=200):
"""
Helper method for sending a patch to the server, defaulting to application/merge-patch+json content_type.
@@ -276,7 +286,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
Verify that all account fields are returned (even those that are not shareable).
"""
data = response.data
assert 29 == len(data)
assert 30 == len(data)
# public fields (3)
expected_account_privacy = (
@@ -417,7 +427,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.create_mock_profile(self.user)
with self.assertNumQueries(26):
with self.assertNumQueries(27):
response = self.send_get(self.different_client)
self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY)
@@ -432,7 +442,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.create_mock_profile(self.user)
with self.assertNumQueries(26):
with self.assertNumQueries(27):
response = self.send_get(self.different_client)
self._verify_private_account_response(response)
@@ -556,7 +566,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
with self.assertNumQueries(queries):
response = self.send_get(self.client)
data = response.data
assert 29 == len(data)
assert 30 == len(data)
assert self.user.username == data['username']
assert ((self.user.first_name + ' ') + self.user.last_name) == data['name']
for empty_field in ("year_of_birth", "level_of_education", "mailing_address", "bio"):
@@ -579,12 +589,12 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
assert data['accomplishments_shared'] is False
self.client.login(username=self.user.username, password=TEST_PASSWORD)
verify_get_own_information(24)
verify_get_own_information(25)
# Now make sure that the user can get the same information, even if not active
self.user.is_active = False
self.user.save()
verify_get_own_information(15)
verify_get_own_information(16)
def test_get_account_empty_string(self):
"""
@@ -599,7 +609,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
legacy_profile.save()
self.client.login(username=self.user.username, password=TEST_PASSWORD)
with self.assertNumQueries(24):
with self.assertNumQueries(25):
response = self.send_get(self.client)
for empty_field in ("level_of_education", "gender", "country", "state", "bio",):
assert response.data[empty_field] is None
@@ -955,7 +965,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
response = self.send_get(client)
if has_full_access:
data = response.data
assert 29 == len(data)
assert 30 == len(data)
assert self.user.username == data['username']
assert ((self.user.first_name + ' ') + self.user.last_name) == data['name']
assert self.user.email == data['email']
@@ -1014,6 +1024,54 @@ class TestAccountAPITransactions(TransactionTestCase):
assert 'm' == data['gender']
@ddt.ddt
class NameChangeViewTests(UserAPITestCase):
""" NameChangeView tests """
def setUp(self):
super().setUp()
self.url = reverse('name_change')
def test_request_succeeds(self):
"""
Test that a valid name change request succeeds.
"""
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self.send_post(self.client, {'name': 'New Name'})
def test_unauthenticated(self):
"""
Test that a name change request fails for an unauthenticated user.
"""
self.send_post(self.client, {'name': 'New Name'}, expected_status=401)
def test_empty_request(self):
"""
Test that an empty request fails.
"""
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self.send_post(self.client, {}, expected_status=400)
def test_blank_name(self):
"""
Test that a blank name string fails.
"""
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self.send_post(self.client, {'name': ''}, expected_status=400)
@ddt.data('<html>invalid name</html>', 'https://invalid.com')
def test_fails_validation(self, invalid_name):
"""
Test that an invalid name will return an error.
"""
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self.send_post(
self.client,
{'name': invalid_name},
expected_status=400
)
@ddt.ddt
@mock.patch('django.conf.settings.USERNAME_REPLACEMENT_WORKER', 'test_replace_username_service_worker')
class UsernameReplacementViewTests(APITestCase):

View File

@@ -55,6 +55,7 @@ from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable=
get_retired_username_by_username,
is_username_retired
)
from common.djangoapps.student.models_api import do_name_change_request
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
@@ -79,6 +80,7 @@ from ..models import (
from .api import get_account_settings, update_account_settings
from .permissions import CanDeactivateUser, CanReplaceUsername, CanRetireUser
from .serializers import (
PendingNameChangeSerializer,
UserRetirementPartnerReportSerializer,
UserRetirementStatusSerializer,
UserSearchEmailSerializer
@@ -246,6 +248,9 @@ class AccountViewSet(ViewSet):
* 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.
* is_verified_name_enabled: Temporary flag to control verified name field - see
https://github.com/edx/edx-name-affirmation/blob/main/edx_name_affirmation/toggles.py
@@ -416,6 +421,42 @@ class AccountViewSet(ViewSet):
return Response(account_settings)
class NameChangeView(APIView):
"""
Request a profile name change. This creates a PendingNameChange to be verified later,
rather than updating the user's profile name directly.
"""
authentication_classes = (JwtAuthentication, SessionAuthentication,)
permission_classes = (permissions.IsAuthenticated,)
def post(self, request):
"""
POST /api/user/v1/accounts/name_change/
Example request:
{
"name": "Jon Doe"
}
"""
user = request.user
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})
if serializer.is_valid():
pending_name_change = do_name_change_request(user, new_name, rationale)[0]
if pending_name_change:
return Response(status=status.HTTP_201_CREATED)
else:
return Response(
'The 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)
class AccountDeactivationView(APIView):
"""
Account deactivation viewset. Currently only supports POST requests.

View File

@@ -16,6 +16,7 @@ from .accounts.views import (
AccountViewSet,
DeactivateLogoutView,
LMSAccountRetirementView,
NameChangeView,
UsernameReplacementView
)
from . import views as user_api_views
@@ -117,6 +118,11 @@ urlpatterns = [
DeactivateLogoutView.as_view(),
name='deactivate_logout'
),
url(
r'^v1/accounts/name_change/$',
NameChangeView.as_view(),
name='name_change'
),
url(
fr'^v1/accounts/{settings.USERNAME_PATTERN}/verification_status/$',
IDVerificationStatusView.as_view(),