Andal.LND/edx-platform
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: Remove Use of VERIFIED_NAME_FLAG Waffle Flag and is_verified_enabled Utility

Browse Source 
The VERIFIED_NAME_FLAG, the VerifiedNameEnabledView, and the verified_name_enabled key removed from responses for both VerifiedNameView view and VerifiedNameHistoryView
were removed as part https://github.com/edx/edx-name-affirmation/pull/12. This was released in version 2.0.0 of the edx-name-affirmation PyPI package. Please see below for additional context for the removal, copied from the name-affirmation commit message.

The VERIFIED_NAME_FLAG was added as part https://github.com/edx/edx-name-affirmation/pull/12, [MST-801](https://openedx.atlassian.net/browse/MST-801) in order to control the release of the Verified Name project. It was used for a phased roll out by percentage of users.

The release reached a percentage of 50% before it was observed that, due to the way percentage roll out works in django-waffle, the code to create or update VerifiedName records was not working properly. The code was written such that any change to a SoftwareSecurePhotoVerification model instance sent a signal, which was received and handled by the Name Affirmation application. If the VERIFIED_NAME_FLAG was on for the requesting user, a Celery task was launched from the Name Affirmation application to perform the creation of or update to the appropriate VerifiedName model instances based on the verify_student application signal. However, we observed that when SoftwareSecurePhotoVerification records were moved into the "created" or "ready" status, a Celery task in Name Affirmation was created, but when SoftwareSecurePhotoVerification records were moved into the "submitted" status, the corresponding Celery task in Name Affirmation was not created. This caused VerifiedName records to stay in the "pending" state.

The django-waffle waffle flag used by the edx-toggle library implements percentage rollout by setting a cookie in a learner's browser session to assign them to the enabled or disabled group.
It turns out that the code that submits a SoftwareSecurePhotoVerification record, which moves it into the "submitted" state, happens as part of a Celery task in the verify_student application in the edx-platform. Therefore, we believe that because there is no request object in a Celery task, the edx-toggle code is defaulting to the case where there is no request object. In this case, the code checks whether the flag is enabled for everyone when determining whether the flag is enabled. Because of the percentage rollout (i.e. waffle flag not enabled for everyone), the Celery task in Name Affirmation is not created. This behavior was confirmed by logging added as part of https://github.com/edx/edx-name-affirmation/pull/62.

We have determined that we do not need the waffle flag, as we are comfortable that enabling the waffle flag for everyone will fix the issue and are comfortable releasing the feature to all users. For this reason, we are removing references to the flag.

[MST-1130](https://openedx.atlassian.net/browse/MST-1130)
This commit is contained in:
michaelroytman
2021-10-27 14:24:12 -04:00
parent d70ebe6343
commit bb299c9521
16 changed files with 31 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
openedx/core/djangoapps/user_api/accounts/api.py
View File

@@ -12,7 +12,6 @@ from django.core.validators import ValidationError, validate_email
from django.utils.translation import override as override_language
from django.utils.translation import gettext as _
from edx_name_affirmation.name_change_validator import NameChangeValidator
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 (
@@ -281,10 +280,7 @@ def _does_name_change_require_verification(user_profile, old_name, new_name):
validator = NameChangeValidator(old_names_list, num_certs, old_name, new_name)
return (
is_verified_name_enabled()
and not validator.validate()
)
return not validator.validate()
def _get_old_language_proficiencies_if_updating(user_profile, data):

2
openedx/core/djangoapps/user_api/accounts/serializers.py
View File

@@ -13,7 +13,6 @@ from django.core.exceptions import ObjectDoesNotExist
from django.urls import reverse
from rest_framework import serializers
from edx_name_affirmation.toggles import is_verified_name_enabled
from common.djangoapps.student.models import (
LanguageProficiency,
@@ -170,7 +169,6 @@ class UserReadOnlySerializer(serializers.Serializer): # lint-amnesty, pylint: d
"extended_profile_fields": None,
"phone_number": None,
"pending_name_change": None,
"is_verified_name_enabled": is_verified_name_enabled(),
}
if user_profile:

6
openedx/core/djangoapps/user_api/accounts/tests/test_api.py
View File

@@ -17,8 +17,6 @@ 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 pytz import UTC
from social_django.models import UserSocialAuth
from common.djangoapps.student.models import (
@@ -365,7 +363,6 @@ 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)
def test_validate_name_change_same_name(self):
"""
Test that saving the user's profile name without changing it should not raise an error.
@@ -396,7 +393,6 @@ 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=False))
@override_waffle_flag(VERIFIED_NAME_FLAG, active=True)
def test_name_update_requires_idv(self):
"""
Test that a name change is blocked through this API if it requires ID verification.
@@ -413,7 +409,6 @@ 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))
@override_waffle_flag(VERIFIED_NAME_FLAG, active=True)
def test_name_update_does_not_require_idv(self):
"""
Test that the user can change their name if change does not require IDV.
@@ -618,7 +613,6 @@ class AccountSettingsOnCreationTest(CreateAccountMixin, TestCase):
'course_certificates': None,
'phone_number': None,
'pending_name_change': None,
'is_verified_name_enabled': False,
}
def test_normalize_password(self):

14
openedx/core/djangoapps/user_api/accounts/tests/test_views.py
View File

@@ -286,7 +286,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
Verify that all account fields are returned (even those that are not shareable).
"""
data = response.data
assert 30 == len(data)
assert 29 == len(data)
# public fields (3)
expected_account_privacy = (
@@ -427,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(27):
with self.assertNumQueries(26):
response = self.send_get(self.different_client)
self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY)
@@ -442,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(27):
with self.assertNumQueries(26):
response = self.send_get(self.different_client)
self._verify_private_account_response(response)
@@ -566,7 +566,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
with self.assertNumQueries(queries):
response = self.send_get(self.client)
data = response.data
assert 30 == len(data)
assert 29 == 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"):
@@ -589,7 +589,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
assert data['accomplishments_shared'] is False
self.client.login(username=self.user.username, password=TEST_PASSWORD)
verify_get_own_information(25)
verify_get_own_information(24)
# Now make sure that the user can get the same information, even if not active
self.user.is_active = False
@@ -609,7 +609,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
legacy_profile.save()
self.client.login(username=self.user.username, password=TEST_PASSWORD)
with self.assertNumQueries(25):
with self.assertNumQueries(24):
response = self.send_get(self.client)
for empty_field in ("level_of_education", "gender", "country", "state", "bio",):
assert response.data[empty_field] is None
@@ -965,7 +965,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
response = self.send_get(client)
if has_full_access:
data = response.data
assert 30 == len(data)
assert 29 == 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']

3
openedx/core/djangoapps/user_api/accounts/views.py
View File

@@ -251,9 +251,6 @@ class AccountViewSet(ViewSet):
* 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
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.