From 1288d964b82c54a037315e672adb5aa68b1b0e8c Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Wed, 8 Sep 2021 10:25:30 -0400 Subject: [PATCH 1/4] docs: clarify docs for _update_full_name --- lms/djangoapps/verify_student/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index ee6c3fd95d..84b4b36461 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -948,7 +948,7 @@ class SubmitPhotosView(View): full_name (unicode): The user's updated full name. Returns: - HttpResponse or None + error encoded as an HttpResponse or None indicating success """ try: From f158fedd15c302b515d55413d11e0994eadb5541 Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Wed, 8 Sep 2021 10:24:37 -0400 Subject: [PATCH 2/4] feat: only update full name if not verified name enabled MST-1015 --- .../verify_student/tests/test_views.py | 30 +++++++++++-------- lms/djangoapps/verify_student/views.py | 3 +- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 5c7a214fb3..a7a88b9c8c 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -21,8 +21,9 @@ from django.test.client import Client, RequestFactory from django.test.utils import override_settings from django.urls import reverse from django.utils.timezone import now +from edx_name_affirmation.toggles import VERIFIED_NAME_FLAG from opaque_keys.edx.locator import CourseLocator -from waffle.testutils import override_switch +from waffle.testutils import override_flag, override_switch from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory @@ -1256,16 +1257,18 @@ class TestSubmitPhotosForVerification(MockS3BotoMixin, TestVerificationBase): # Verify that the user's name wasn't changed self._assert_user_name(self.user.profile.name) - def test_submit_photos_and_change_name(self): - # Submit the photos, along with a name change - self._submit_photos( - face_image=self.IMAGE_DATA, - photo_id_image=self.IMAGE_DATA, - full_name=self.FULL_NAME - ) + @ddt.data(True, False) + def test_submit_photos_and_change_name(self, flag_on): + with override_flag(VERIFIED_NAME_FLAG.name, flag_on): + # Submit the photos, along with a name change + self._submit_photos( + face_image=self.IMAGE_DATA, + photo_id_image=self.IMAGE_DATA, + full_name=self.FULL_NAME + ) - # Check that the user's name was changed in the database - self._assert_user_name(self.FULL_NAME) + # Check that the user's name was changed in the database if verified_name is off + self._assert_user_name(self.FULL_NAME, equality=not flag_on) def test_submit_photos_sends_confirmation_email(self): self._submit_photos( @@ -1479,7 +1482,7 @@ class TestSubmitPhotosForVerification(MockS3BotoMixin, TestVerificationBase): # Verify that photo submission confirmation email was not sent assert len(mail.outbox) == 0 - def _assert_user_name(self, full_name): + def _assert_user_name(self, full_name, equality=True): """Check the user's name. Arguments: @@ -1492,7 +1495,10 @@ class TestSubmitPhotosForVerification(MockS3BotoMixin, TestVerificationBase): request = RequestFactory().get('/url') request.user = self.user account_settings = get_account_settings(request)[0] - assert account_settings['name'] == full_name + if equality: + assert account_settings['name'] == full_name + else: + assert not account_settings['name'] == full_name def _get_post_data(self): """Retrieve POST data from the last request. """ diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 84b4b36461..b0d919fd91 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -21,6 +21,7 @@ from django.utils.translation import ugettext_lazy from django.views.decorators.csrf import csrf_exempt from django.views.decorators.http import require_POST from django.views.generic.base import View +from edx_name_affirmation.toggles import is_verified_name_enabled from edx_rest_api_client.exceptions import SlumberBaseException from ipware.ip import get_client_ip from opaque_keys.edx.keys import CourseKey @@ -846,7 +847,7 @@ class SubmitPhotosView(View): return response # If necessary, update the user's full name - if "full_name" in params: + if "full_name" in params and not is_verified_name_enabled(): response = self._update_full_name(request, params["full_name"]) if response is not None: return response From 695f2cd4c875849720bf677d5e3c95f891d4d3a9 Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Wed, 8 Sep 2021 13:15:48 -0400 Subject: [PATCH 3/4] feat: prevent name update on photo verification if the name is already updated MST-1015 --- lms/djangoapps/verify_student/models.py | 8 ++++---- lms/djangoapps/verify_student/tests/test_models.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 02a865d4f0..063a501096 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/lms/djangoapps/verify_student/models.py @@ -398,10 +398,10 @@ class PhotoVerification(IDVerificationAttempt): they uploaded are good. Note that we don't actually do a submission anywhere yet. """ - # At any point prior to this, they can change their names via their - # student dashboard. But at this point, we lock the value into the - # attempt. - self.name = self.user.profile.name # pylint: disable=no-member + # If a name is not already set via the verified_name flow, + # pick up the profile name at this time. + if not self.name: + self.name = self.user.profile.name # pylint: disable=no-member self.status = self.STATUS.ready self.save() diff --git a/lms/djangoapps/verify_student/tests/test_models.py b/lms/djangoapps/verify_student/tests/test_models.py index 268b48bbc1..efc26788f7 100644 --- a/lms/djangoapps/verify_student/tests/test_models.py +++ b/lms/djangoapps/verify_student/tests/test_models.py @@ -185,6 +185,20 @@ class TestPhotoVerification(TestVerificationBase, MockS3BotoMixin, ModuleStoreTe assert 'Clyde ƴ' == attempt.name + def test_name_preset(self): + """ + If a name was set when creating the photo verification + (from name affirmation / verified name flow) it should not + be overwritten by the profile name + """ + user = UserFactory.create() + user.profile.name = "Profile" + + preset_attempt = SoftwareSecurePhotoVerification(user=user) + preset_attempt.name = "Preset" + preset_attempt.mark_ready() + assert "Preset" == preset_attempt.name + def test_submissions(self): """Test that we set our status correctly after a submission.""" # Basic case, things go well. From dc01bf3aadf60cebe90af46b2cccc20deb5047ce Mon Sep 17 00:00:00 2001 From: Andy Shultz Date: Wed, 8 Sep 2021 13:46:44 -0400 Subject: [PATCH 4/4] feat: add parameter to submit attempt to carry full name through original behavior does pass the empty name through to _update_full_name rather than just considering that as full name not set. That's a little weird but outside the scope of this work so I've preserved it by checking is not None rather than just using full_name as a boolean. MST-1015 --- lms/djangoapps/verify_student/tests/test_views.py | 4 ++++ lms/djangoapps/verify_student/views.py | 15 +++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index a7a88b9c8c..cb0c1f8e61 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1269,6 +1269,10 @@ class TestSubmitPhotosForVerification(MockS3BotoMixin, TestVerificationBase): # Check that the user's name was changed in the database if verified_name is off self._assert_user_name(self.FULL_NAME, equality=not flag_on) + # Since we are giving a full name, it should be written into the attempt + # whether or not the user name was updated + attempt = SoftwareSecurePhotoVerification.objects.get(user=self.user) + self.assertEqual(attempt.name, self.FULL_NAME) def test_submit_photos_sends_confirmation_email(self): self._submit_photos( diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index b0d919fd91..79fff495dd 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -846,9 +846,13 @@ class SubmitPhotosView(View): if response is not None: return response + full_name = None + if "full_name" in params: + full_name = params["full_name"] + # If necessary, update the user's full name - if "full_name" in params and not is_verified_name_enabled(): - response = self._update_full_name(request, params["full_name"]) + if full_name is not None and not is_verified_name_enabled(): + response = self._update_full_name(request, full_name) if response is not None: return response @@ -867,7 +871,7 @@ class SubmitPhotosView(View): return response # Submit the attempt - attempt = self._submit_attempt(request.user, face_image, photo_id_image, initial_verification) + attempt = self._submit_attempt(request.user, face_image, photo_id_image, initial_verification, full_name) # Send event to segment for analyzing A/B testing data data = { @@ -1019,7 +1023,7 @@ class SubmitPhotosView(View): log.error(("Image data for user {user_id} is not valid").format(user_id=request.user.id)) return None, None, HttpResponseBadRequest(msg) - def _submit_attempt(self, user, face_image, photo_id_image=None, initial_verification=None): + def _submit_attempt(self, user, face_image, photo_id_image=None, initial_verification=None, provided_name=None): """ Submit a verification attempt. @@ -1030,8 +1034,11 @@ class SubmitPhotosView(View): Keyword Arguments: photo_id_image (str or None): Decoded photo ID image data. initial_verification (SoftwareSecurePhotoVerification): The initial verification attempt. + provided_name (str or None): full name given by user for this attempt """ attempt = SoftwareSecurePhotoVerification(user=user) + if provided_name: + attempt.name = provided_name # We will always have face image data, so upload the face image attempt.upload_face_image(face_image)